Closed Bug 995162 Opened 10 years ago Closed 10 years ago

TypeError: worker is null

Categories

(Toolkit Graveyard :: OS.File, defect)

defect
Not set
normal

Tracking

(firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Seen on Windows and OSX, but not Linux, for talos-other here:
https://tbpl.mozilla.org/?tree=Try&rev=64bd9134ae15
Attached patch Making |worker| more robust (obsolete) — Splinter Review
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
(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.
Attached patch Making |worker| more robust, v2 (obsolete) — Splinter Review
Same one, minus typo.
Try: https://tbpl.mozilla.org/?tree=Try&rev=6b86552c486d
Attachment #8405391 - Attachment is obsolete: true
Attachment #8405570 - Flags: review?(nfroyd)
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 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)
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.
Flags: needinfo?(nfroyd)
(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)
(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)
(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 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+
Same one, with a commit message.
Attachment #8405570 - Attachment is obsolete: true
Attachment #8407123 - Flags: review+
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
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?
Attachment #8407123 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [qa-]
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.