Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(6)

Issue 29338242: Issue 3792 - Fix to support multiprocess firefox (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 6 months ago by sergei
Modified:
2 years, 11 months ago
Reviewers:
Wladimir Palant
CC:
Thomas Greiner, tschuster
Visibility:
Public.

Description

# abpcrawler project # Tested on released Firefox 47.

Patch Set 1 #

Total comments: 17

Patch Set 2 : rebase on #3815 and address some trivial comments #

Patch Set 3 : fix issue with beacons #

Patch Set 4 : add missed space #

Patch Set 5 : rebase, fix race condition and support canvas limits #

Patch Set 6 : fix race condition #

Total comments: 42

Patch Set 7 : address comments #

Total comments: 19

Patch Set 8 : address comments #

Patch Set 9 : fix #

Total comments: 2

Patch Set 10 : change comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+180 lines, -106 lines) Patch
A lib/child/frameScript.js View 1 2 3 4 5 6 7 8 9 1 chunk +129 lines, -0 lines 0 comments Download
M lib/crawler.js View 1 2 3 4 5 6 7 8 9 10 chunks +51 lines, -106 lines 0 comments Download

Messages

Total messages: 15
sergei
3 years, 6 months ago (2016-03-14 14:47:03 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29338242/diff/29338243/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29338243/lib/child/frameScript.js#newcode1 lib/child/frameScript.js:1: "use strict"; We put "use strict" after the license ...
3 years, 6 months ago (2016-03-15 10:07:11 UTC) #2
sergei
So far merely a couple of comments. https://codereview.adblockplus.org/29338242/diff/29338243/lib/crawler.js File lib/crawler.js (left): https://codereview.adblockplus.org/29338242/diff/29338243/lib/crawler.js#oldcode157 lib/crawler.js:157: }; On ...
3 years, 6 months ago (2016-03-15 16:40:11 UTC) #3
sergei
https://codereview.adblockplus.org/29338242/diff/29338243/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29338243/lib/child/frameScript.js#newcode1 lib/child/frameScript.js:1: "use strict"; On 2016/03/15 10:07:10, Wladimir Palant wrote: > ...
3 years, 6 months ago (2016-03-16 14:44:23 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29338242/diff/29338243/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338242/diff/29338243/lib/crawler.js#newcode265 lib/crawler.js:265: reportException(msg.objects); On 2016/03/16 14:44:23, sergei wrote: > Just wonder, ...
3 years ago (2016-09-14 16:11:49 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29338242/diff/29340782/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338242/diff/29340782/lib/crawler.js#newcode13 lib/crawler.js:13: Cu.import("resource://gre/modules/Promise.jsm"); Just realized that this import hasn't been removed ...
3 years ago (2016-09-14 16:13:18 UTC) #6
sergei
https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js#newcode6 lib/child/frameScript.js:6: "use strict"; On 2016/09/14 16:11:47, Wladimir Palant wrote: > ...
2 years, 11 months ago (2016-09-29 09:58:14 UTC) #7
Wladimir Palant
https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js#newcode53 lib/child/frameScript.js:53: return; On 2016/09/29 09:58:13, sergei wrote: > It was ...
2 years, 11 months ago (2016-09-29 11:44:59 UTC) #8
sergei
https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js#newcode53 lib/child/frameScript.js:53: return; On 2016/09/29 11:44:57, Wladimir Palant wrote: > On ...
2 years, 11 months ago (2016-09-29 15:36:23 UTC) #9
Wladimir Palant
LGTM but please improve the comment as outlined below. https://codereview.adblockplus.org/29338242/diff/29355263/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29355263/lib/child/frameScript.js#newcode43 lib/child/frameScript.js:43: ...
2 years, 11 months ago (2016-09-30 07:43:13 UTC) #10
sergei
https://codereview.adblockplus.org/29338242/diff/29355374/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29355374/lib/child/frameScript.js#newcode45 lib/child/frameScript.js:45: // Another not interesting for us case is about:newtab. ...
2 years, 11 months ago (2016-09-30 13:27:24 UTC) #11
Wladimir Palant
LGTM
2 years, 11 months ago (2016-09-30 20:27:27 UTC) #12
sergei
https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js#newcode53 lib/child/frameScript.js:53: return; On 2016/09/29 15:36:21, sergei wrote: > On 2016/09/29 ...
2 years, 11 months ago (2016-10-04 10:57:32 UTC) #13
Wladimir Palant
https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js File lib/child/frameScript.js (right): https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScript.js#newcode53 lib/child/frameScript.js:53: return; On 2016/10/04 10:57:31, sergei wrote: > What about ...
2 years, 11 months ago (2016-10-04 11:10:38 UTC) #14
sergei
2 years, 11 months ago (2016-10-06 14:26:59 UTC) #15
Message was sent while issue was closed.
https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScri...
File lib/child/frameScript.js (right):

https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScri...
lib/child/frameScript.js:53: return;
On 2016/10/04 11:10:38, Wladimir Palant wrote:
> On 2016/10/04 10:57:31, sergei wrote:
> > What about creation of a new tab in TabAllocator.releaseTab?
> 
> No, I'd rather not make the code more complicated than it already is for this
> edge case. I'd suggest that you push your patch and deal with it in a
follow-up
> issue - this took long enough already.
Of course it should be in a follow-up issue, the question is whether we need it
or not.
> 
> > When we navigate to about:blank from our URL we get STATE_STOP the third
time
> but the value of content.location.href is sometimes (!) still old (our URL)
> 
> Sure that this is STATE_STOP for about:blank? I'd suspect that the page timed
> out, so it's actually getting its first STATE_STOP when you navigate away to
> about:blank - and content.location.href is set correctly at that point. I'm
not
> sure why creating a screenshot fails at that point but maybe
document.readyState
> will help you recognize this situation.

Yes, sure, there is simply no space to time out. Here is the log when we get
STATE_STOP and the number of tabs is 1.
console.log: owinID: 14, href: about:blank, rstate: uninitialized
console.log: owinID: 16, href: about:newtab, rstate: complete
console.log: owinID: 14, href: ftp://speedtest.tele2.net/, rstate: complete
console.log: owinID: 14, href: ftp://speedtest.tele2.net/, rstate: complete
console.log: owinID: 20, href: about:blank, rstate: uninitialized
console.log: owinID: 20, href: https://www.amazon.com/, rstate: complete
console.log: owinID: 20, href: https://www.amazon.com/, rstate: complete
console.log: owinID: 27, href: about:blank, rstate: uninitialized
console.log: owinID: 27, href: https://codereview.adblockplus.org/, rstate:
complete
console.log: owinID: 27, href: https://codereview.adblockplus.org/, rstate:
complete
console.log: owinID: 30, href: about:blank, rstate: uninitialized
console.log: owinID: 30, href: https://twitter.com/, rstate: complete
console.log: owinID: 30, href: about:blank, rstate: complete
console.log: owinID: 43, href: about:blank, rstate: complete
console.log: owinID: 43, href: http://edition.cnn.com/, rstate: complete
console.log: owinID: 43, href: http://edition.cnn.com/, rstate: complete
console.log: owinID: 94, href: about:blank, rstate: uninitialized
console.log: owinID: 94, href: http://bcc.com/, rstate: complete
console.log: owinID: 94, href: http://bcc.com/, rstate: complete
console.log: owinID: 97, href: about:blank, rstate: uninitialized
console.log: owinID: 97, href: https://adblockplus.org/, rstate: complete
console.log: owinID: 97, href: about:blank, rstate: complete

owinID - outerWindowID
rstate - document.readyState

I have tried it with commented line tab.linkedBrowser.loadURI("about:blank",
null, null); in releaseTab and it looks very similar however there is no any
third entry for the same outer window ID.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5