Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Created:
March 13, 2014, 1:52 p.m. by Wladimir Palant
Modified:
March 14, 2014, 9:47 p.m.
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
March 13, 2014, 1:52 p.m. (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"))); ...
March 14, 2014, 4:10 p.m. (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"))); ...
March 14, 2014, 6:04 p.m. (2014-03-14 18:04:50 UTC) #3
Wladimir Palant
Fixed wrong callback parameters
March 14, 2014, 6:08 p.m. (2014-03-14 18:08:43 UTC) #4
Felix Dahlke
March 14, 2014, 9:32 p.m. (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.

Powered by Google App Engine
This is Rietveld