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

VERIFIED FIXED in Firefox 31

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: lizzard, Assigned: bholley)

Tracking

({crash, topcrash, topcrash-win})

31 Branch
mozilla33
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30 wontfix, firefox31+ verified, firefox32 verified, firefox33 verified, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(crash signature)

Attachments

(2 attachments)

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.
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);
Blocks: 979481
Component: DOM: Core & HTML → DOM: Workers
Er, ignore the part at the bottom of comment 1.  ;)

What this code trying to accomplish in the cases when it uses a SafeJSContext?
Flags: needinfo?(khuey)
Flags: needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
Yes, but handle it how?  Where is it trying to throw/report the exception to?
Flags: needinfo?(bent.mozilla)
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?
Flags: needinfo?(bent.mozilla)
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?
Flags: needinfo?(khuey) → needinfo?(bobbyholley)
Assignee

Comment 7

5 years ago
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.
Flags: needinfo?(bobbyholley)
Tracking this for now since it is a topcrash.
(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?
(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.
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.
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.
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.
Flags: needinfo?(bobbyholley)
OS: Windows NT → All
Assignee

Comment 14

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d4940bd1789e
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
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
Attachment #8437219 - Flags: review?(bzbarsky) → review+
Assignee

Comment 20

5 years ago
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.
Attachment #8437221 - Flags: approval-mozilla-beta?
Attachment #8437221 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/de567b2d9350
https://hg.mozilla.org/mozilla-central/rev/3fe4e53dfe59
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8437221 - Flags: approval-mozilla-beta?
Attachment #8437221 - Flags: approval-mozilla-beta+
Attachment #8437221 - Flags: approval-mozilla-aurora?
Attachment #8437221 - Flags: approval-mozilla-aurora+
Assignee

Comment 24

5 years ago
(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.
Flags: needinfo?(bobbyholley)
(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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.