990090 - crash in JS::DescribeStack(JSContext*, unsigned int)

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[Liz Henry (:lizzard) (relman/hg->git project)]

This bug was filed from the Socorro interface and is 
report bp-f3c4c86d-9316-4f97-9324-a28712140330.
=============================================================
This is the #15 topcrash for Firefox 31 this week, with 65 out of 4290 crashes. It looks like the crashes spiked suddenly with the 2014032703 build.

Comment 1

[Boris Zbarsky [:bzbarsky]]

It's a null deref on cx->compartment()->principals.

We grab the cx in dom::GetCurrentJSStack from nsXPConnect::GetCurrentJSContext(), when on main thread, and the callstack is in fact on main thread, though in worker code: WorkerPrivateParent<Derived>::BroadcastErrorToSharedWorkers asserts IsOnMainThread.'

This runnable does:

3121     if (actionsIndex == windowActions.NoIndex) {
3122       nsIScriptContext* scx = sharedWorker->GetContextForEventHandlers(&rv);
3123       cx = (NS_SUCCEEDED(rv) && scx) ?
3124            scx->GetNativeContext() :
3125            nsContentUtils::GetSafeJSContext();
3126     } else {
3127       cx = windowActions[actionsIndex].mJSContext;
3128     }
3129 
3130     AutoCxPusher autoPush(cx);

Note that this can push the safe JS context, as far as I can see, in the SharedWorker case, without entering a compartment on it.  And then it tries to throw/report an error on there, which presumably runs into the fix for bug 979481.
3128 }
bent@149749
3129
bent@149749
3130 AutoCxPusher autoPush(cx);

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er, ignore the part at the bottom of comment 1.  ;)

What this code trying to accomplish in the cases when it uses a SafeJSContext?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It's trying to handle the case where a SharedWorker lives somewhere without a window, like a JS component or JSM or bootstrap.js file I think.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, but handle it how?  Where is it trying to throw/report the exception to?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, well, presumably to the context's error reporter. For the SafeJSContext I guess it gets dumped to stderr and (maybe) ends up in the Error Console?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I guess the real question is whether there is a spec for SharedWorker error reporting.

I guess what we should be doing here is entering the compartment of xpc::GetSafeJSContextGlobal() in the case when we're using the safe context?

Comment 7

[Bobby Holley (:bholley)]

Seems like it would make more sense just to replace the whole thing with AutoEntryGlobal entry(globalObject). That'll sort out the correct context, push it, and enter the global's compartment.

We should make a note that, depending on what we decided to do about error reporting in bug 989528, AutoEntryGlobal may not be the right long-term structure.

Comment 8

[Benjamin Kerensa [:bkerensa]]

Tracking this for now since it is a topcrash.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> I guess the real question is whether there is a spec for SharedWorker error
> reporting.

Hixie was hesitant to spec since only Chrome had SharedWorker previously, but we chatted on irc and he said he had no objections to this approach.

It seems crazy to me that this is a topcrash... Who is using SharedWorker in a context where there's no window?

Comment 10

[Jan Alexander Steffens [:heftig]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #9)
> It seems crazy to me that this is a topcrash... Who is using SharedWorker in
> a context where there's no window?

I'm seeing a lot of crashes in DescribeStack on https://www.fimfiction.net, which (AFAIK) uses SharedWorkers for its web notifications.

Comment 11

[Jan Alexander Steffens [:heftig]]

Oh, and while this bug is about Windows, for me this happens in both Firefox 31.0a2 for Linux (Desktop) and Firefox 31.0a1 for Android. I've submitted crash reports for the latter, and bp-601d59a6-bbe8-4fa5-9414-afbac2140429 should be one of mine.

For now I've turned off shared workers as a workaround.

Comment 12

[Jan Alexander Steffens [:heftig]]

Talked to one of Fimfiction's developer's about this bug, and it was discovered that the bug most likely happens in this scenario:

There's only one tab making use of the notifications worker and the user navigates. This releases the worker and the window. When the worker now tries to make an update before it is destroyed, it throws an exception as there is no page to be updated and Firefox crashes as there is no window to report the error to.

When there are at least two Fimfiction tabs open, the other tab will hold onto the worker so no crash happens.

This is the worker code: http://www.fimfiction.net/workers/notifications_worker.js

For anyone trying to replicate this, you'll need an account. Otherwise there won't be a worker.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Bobby, could you have a look to this bug or propose someone who could fix it? 31 is going to be in beta next week and it would be nice to have a fix. Thanks


As comment 11 says, it seems to affect other platforms.

Comment 14

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=d4940bd1789e

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add a version of AutoJSAPIWithErrorsReportedToWindow that takes an nsIGlobalObject. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use AutoJSAPI for BroadcastErrorToSharedWorkers. v1

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8437219 [details] [diff] [review]
Part 1 - Add a version of AutoJSAPIWithErrorsReportedToWindow that takes an nsIGlobalObject. v1

>+    // Equivalent to AutoJSAPI is aGlobal is not a Window.

s/is aGlobal/if aGlobal/

r=me

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

This crash is still present in Firefox 30, 31, 32, and 33. 

https://crash-stats.mozilla.com/report/list?signature=JS%3A%3ADescribeStack%28JSContext*%2C+unsigned+int%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2014-06-12+16%3A00%3A00&range_value=1#tab-reports

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de567b2d9350
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe4e53dfe59

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8437221 [details] [diff] [review]
Part 2 - Use AutoJSAPI for BroadcastErrorToSharedWorkers. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: crashes
Testing completed (on m-c, etc.): Just pushed to m-i
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de567b2d9350
https://hg.mozilla.org/mozilla-central/rev/3fe4e53dfe59

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f436868418ee
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1aebc2f1c58

https://hg.mozilla.org/releases/mozilla-beta/rev/4c62cbb21c01
https://hg.mozilla.org/releases/mozilla-beta/rev/86f90cef7574

Comment 23

[Bogdan Maris, Desktop QA]

Nightly 0 crashes in the last week:
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A33.0a1&range_value=1&range_unit=weeks&date=07%2F01%2F2014+11%3A00%3A00&query_search=signature&query_type=contains&query=JS%3A%3ADescribeStack%28JSContext*%2C+unsigned+int%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

Aurora 0 crashes in the last week:
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A32.0a2&range_value=1&range_unit=weeks&date=07%2F01%2F2014+11%3A00%3A00&query_search=signature&query_type=contains&query=JS%3A%3ADescribeStack%28JSContext*%2C+unsigned+int%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

Beta 11 crashes in the last week:
https://crash-stats.mozilla.com/report/list?signature=JS%3A%3ADescribeStack%28JSContext%2A%2C+unsigned+int%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A31.0b&hang_type=any&date=2014-07-01+11%3A00%3A00&range_value=1#reports

Crashes after the fix:
31 beta 2 - 2 crashes
31 beta 4 - 3 crashes
31 beta 5 - 3 crashes

Can we say that is verified on 31 regarding the low number of crashes recorded?

Comment 24

[Bobby Holley (:bholley)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #23)
> Can we say that is verified on 31 regarding the low number of crashes
> recorded?

That sounds fine to me.

Comment 25

[Bogdan Maris, Desktop QA]

(In reply to Bobby Holley (:bholley) from comment #24)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #23)
> > Can we say that is verified on 31 regarding the low number of crashes
> > recorded?
> 
> That sounds fine to me.

Thanks, marking as verified on 31.