Closed
Bug 995162
Opened 11 years ago
Closed 11 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•11 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•11 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•11 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•11 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•11 years ago
|
Keywords: regression
![]() |
||
Comment 5•11 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•11 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•11 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•11 years ago
|
Flags: needinfo?(nfroyd)
![]() |
||
Comment 8•11 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•11 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•11 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•11 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•11 years ago
|
||
Same one, with a commit message.
Attachment #8405570 -
Attachment is obsolete: true
Attachment #8407123 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 13•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 14•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla31
Assignee | ||
Comment 15•11 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•11 years ago
|
Attachment #8407123 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•11 years ago
|
||
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Comment 17•11 years ago
|
||
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
Updated•2 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•