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

Issue 5731880649359360: Don't delay Firefox startup (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by Wladimir Palant
Modified:
5 years, 7 months ago
Reviewers:
Felix Dahlke
Visibility:
Public.

Description

This fixes https://issues.adblockplus.org/ticket/117. The critical part is really dealing with potential reentrance. Originally I wanted to add reentrance protection to INIParser but that would allow the "done" notification to happen before all lines are processed. So it seems that preventing reentrance in IO.readFromFile() is the best option.

Patch Set 1 #

Total comments: 9

Patch Set 2 : Fixed wrong callback parameters #

Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -16 lines) Patch
M lib/filterStorage.js View 3 chunks +9 lines, -1 line 0 comments Download
M lib/io.js View 1 1 chunk +64 lines, -15 lines 0 comments Download
M lib/utils.js View 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 5
Wladimir Palant
5 years, 7 months ago (2014-03-13 13:52:57 UTC) #1
Felix Dahlke
http://codereview.adblockplus.org/5731880649359360/diff/5629499534213120/lib/io.js File lib/io.js (right): http://codereview.adblockplus.org/5731880649359360/diff/5629499534213120/lib/io.js#newcode92 lib/io.js:92: let index = (processing ? -1 : Math.max(data.lastIndexOf("\n"), data.lastIndexOf("\r"))); ...
5 years, 7 months ago (2014-03-14 16:10:45 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/5731880649359360/diff/5629499534213120/lib/io.js File lib/io.js (right): http://codereview.adblockplus.org/5731880649359360/diff/5629499534213120/lib/io.js#newcode92 lib/io.js:92: let index = (processing ? -1 : Math.max(data.lastIndexOf("\n"), data.lastIndexOf("\r"))); ...
5 years, 7 months ago (2014-03-14 18:04:50 UTC) #3
Wladimir Palant
Fixed wrong callback parameters
5 years, 7 months ago (2014-03-14 18:08:43 UTC) #4
Felix Dahlke
5 years, 7 months ago (2014-03-14 21:32:49 UTC) #5
LGTM

http://codereview.adblockplus.org/5731880649359360/diff/5629499534213120/lib/...
File lib/io.js (right):

http://codereview.adblockplus.org/5731880649359360/diff/5629499534213120/lib/...
lib/io.js:92: let index = (processing ? -1 : Math.max(data.lastIndexOf("\n"),
data.lastIndexOf("\r")));
On 2014/03/14 18:04:51, Wladimir Palant wrote:
> On 2014/03/14 16:10:45, Felix H. Dahlke wrote:
> > This doesn't really seem reentrant to me. What if "processing" is false
here,
> > but actually gets set to true before we try to set it to true below?
> 
> JavaScript is a single-threaded language, reentrance cannot happen at a random
> time. A call to Utils.yield() will spin the event loop however, which means
that
> events waiting to be processed will be processed. That call will return only
> once all the events have been removed from the queue.
> 
> One such event can be XMLHttpRequest's progress event. So while we are in the
> call to listener.process(...) this event handler can be called again (or the
> handler for the "load" or "error" events) if the listener calls Utils.yield().

Ah, thought this was actually proper multithreading. But yeah, currentThread
pretty much implies that it's not. So listener.process() can call Utils.yield()
which can in turn call onProcess() while we're still in the middle of
processing. Got it.
Sign in to reply to this message.

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