995162 - TypeError: worker is null

Status: RESOLVED FIXED

(Toolkit :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Seen in different bugs.
https://tbpl.mozilla.org/php/getParsedLog.php?id=37619720&tree=Try&full=1#error0

Comment 1

[Georg Fritzsche [:gfritzsche]]

Seen on Windows and OSX, but not Linux, for talos-other here:
https://tbpl.mozilla.org/?tree=Try&rev=64bd9134ae15

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: 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.

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

(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.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Making |worker| more robust, v2

Same one, minus typo.
Try: https://tbpl.mozilla.org/?tree=Try&rev=6b86552c486d

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 <[e-mail]>
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

[Nathan Froyd [: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.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 8

[Nathan Froyd [:froydnj]]

(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?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 10

[Nathan Froyd [:froydnj]]

(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.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8405570 [details] [diff] [review]
Making |worker| more robust, v2

r=me with the other nitpicky comments from previous review addressed.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Making |worker| more robust, v3

Same one, with a commit message.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/8bac6ad1d812

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bac6ad1d812

Comment 15

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0678e5469636

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/0678e5469636