Closed
Bug 995162
Opened 10 years ago
Closed 10 years ago
TypeError: worker is null
Categories
(Toolkit Graveyard :: OS.File, defect)
Toolkit Graveyard
OS.File
Tracking
(firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)
RESOLVED
FIXED
mozilla31
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
(Keywords: regression, Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
8.55 KB,
patch
|
Yoric
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Seen in different bugs. https://tbpl.mozilla.org/php/getParsedLog.php?id=37619720&tree=Try&full=1#error0
Comment 1•10 years ago
|
||
Seen on Windows and OSX, but not Linux, for talos-other here: https://tbpl.mozilla.org/?tree=Try&rev=64bd9134ae15
Assignee | ||
Comment 2•10 years ago
|
||
I have the feeling that this is a shutdown-time race condition with client code that should be using AsyncShutdown. However, just in case, here's a patch making |worker| more robust.
Assignee: nobody → dteller
Comment 3•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on April 9th-16th) from comment #2) > Created attachment 8405391 [details] [diff] [review] > Making |worker| more robust > > I have the feeling that this is a shutdown-time race condition with client > code that should be using AsyncShutdown. However, just in case, here's a > patch making |worker| more robust. Thanks; who can we get to review this? I'd like to see if it helps/fixes our blocking situation, over in bug 990580.
Assignee | ||
Comment 4•10 years ago
|
||
Same one, minus typo. Try: https://tbpl.mozilla.org/?tree=Try&rev=6b86552c486d
Attachment #8405391 -
Attachment is obsolete: true
Attachment #8405570 -
Flags: review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Keywords: regression
Comment 5•10 years ago
|
||
Fwiw, this is easily reproducible for me by running "firefox -silent". I bisected that, and came out with: The first bad revision is: changeset: 176776:aa93afa78cd5 user: David Rajchenbach-Teller <dteller@mozilla.com> date: Wed Apr 02 11:42:51 2014 -0400 summary: Bug 961317 - Clean up OS.File shutdown race condition and rework OS.File reset/shutdown. r=froydnj
Comment 6•10 years ago
|
||
Comment on attachment 8405570 [details] [diff] [review] Making |worker| more robust, v2 Review of attachment 8405570 [details] [diff] [review]: ----------------------------------------------------------------- I'm canceling this review so that it's more obvious in your inbox, but everything looks fine apart from the one issue raised below. Re-r? once you describe what you think is going on in a little more detail. ::: toolkit/components/osfile/modules/osfile_async_front.jsm @@ +283,5 @@ > return Task.spawn(function*() { > > yield this.queue; > > + // Enter critical section: no yield in this blog "...in this blog"? Give me parallelism or give me death? Fix this comment, please. @@ +414,5 @@ > + if (this.shutdown) { > + LOG("OS.File is not available anymore. The following request has been rejected.", > + method, args); > + throw new Error("OS.File has been shut down."); > + } I guess we can get here because some previous request to shutdown hadn't been completed when this Task was pushed into the queue? @@ +430,5 @@ > let reply; > let isError = false; > try { > try { > + data = yield this.worker.post(method, ...args); So this is really the only problematic line. All the other uses of |worker| should have worker defined already. How do we get here without a worker? I'm not convinced that starting the worker here if we don't already have one is the best course of action.
Attachment #8405570 -
Flags: review?(nfroyd)
Assignee | ||
Comment 7•10 years ago
|
||
There are two ways to get to that line without a worker: 1/ The first time we get here, the worker hasn't been started yet; 2/ After a call to Scheduler.kill({shutdown: false}), which may be triggered by the |resetTimer| for the purpose of reducing the footprint of the OS.File worker.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nfroyd)
Comment 8•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on April 9th-16th) from comment #7) > 2/ After a call to Scheduler.kill({shutdown: false}), which may be triggered > by the |resetTimer| for the purpose of reducing the footprint of the OS.File > worker. Hm, OK. I don't know if it's worth making everything lazy to address just two cases, which both seem like they have clear points where we could figure out what's going on. WDYT?
Flags: needinfo?(dteller)
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #8) > (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - > I'll be away on April 9th-16th) from comment #7) > > 2/ After a call to Scheduler.kill({shutdown: false}), which may be triggered > > by the |resetTimer| for the purpose of reducing the footprint of the OS.File > > worker. > > Hm, OK. > > I don't know if it's worth making everything lazy to address just two cases, > which both seem like they have clear points where we could figure out what's > going on. WDYT? Well, bug 961317 and the current bug seem to indicate that the clear points are not really that clear. In addition, the semantics of case 2/ is exactly "next time we use a worker, recreate it", which is pretty much the same thing as making it lazy, so I can't really think of a nicer or more self-contained way to make this happen.
Flags: needinfo?(dteller)
Comment 10•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on April 9th-16th) from comment #9) > Well, bug 961317 and the current bug seem to indicate that the clear points > are not really that clear. In addition, the semantics of case 2/ is exactly > "next time we use a worker, recreate it", which is pretty much the same > thing as making it lazy, so I can't really think of a nicer or more > self-contained way to make this happen. Fair, I guess.
Flags: needinfo?(nfroyd)
Comment 11•10 years ago
|
||
Comment on attachment 8405570 [details] [diff] [review] Making |worker| more robust, v2 r=me with the other nitpicky comments from previous review addressed.
Attachment #8405570 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
Same one, with a commit message.
Attachment #8405570 -
Attachment is obsolete: true
Attachment #8407123 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8bac6ad1d812
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8bac6ad1d812
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla31
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8407123 [details] [diff] [review] Making |worker| more robust, v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): 961317 User impact if declined: Not sure. Worrying warnings appear during shutdown, not sure if they have an impact. Testing completed (on m-c, etc.): Has been on m-c (now Aurora) for 14 days. Risk to taking this patch (and alternatives if risky): None that I can think of. String or IDL/UUID changes made by this patch: None.
Attachment #8407123 -
Flags: approval-mozilla-beta?
Updated•10 years ago
|
Attachment #8407123 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/0678e5469636
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/0678e5469636
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
Updated•10 months ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•