1525300 - Crash in JS::GetRealmPrincipals

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P3)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-6ddd7109-9607-473b-b980-d55c10190205.

Seen while reviewing nightly crash data: https://bit.ly/2TxSeOy. This crash seems to be more visible on nightly, and barely visible on release. 11 crashes/9 installs in the last week in 67 nightly.

About 1/2 of the crashes overall appear to be startup crashes.

Top 10 frames of crashing thread:

0 xul.dll JS::GetRealmPrincipals js/src/jsfriendapi.cpp:170
1 xul.dll XPCJSContext::IsSystemCaller js/xpconnect/src/XPCJSContext.cpp:1255
2 xul.dll mozilla::CycleCollectedJSRuntime::ErrorInterceptor::interceptError xpcom/base/CycleCollectedJSRuntime.cpp:1472
3 xul.dll JSContext::setPendingException js/src/vm/JSContext-inl.h:334
4 xul.dll js::ReportOutOfMemory js/src/vm/JSContext.cpp:311
5 xul.dll js::AtomizeChars<char16_t> js/src/vm/JSAtom.cpp:971
6 xul.dll js::frontend::TokenStreamSpecific<char16_t, js::frontend::ParserAnyCharsAccess<js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t> > >::identifierName js/src/frontend/TokenStream.cpp:1983
7 xul.dll js::frontend::TokenStreamSpecific<char16_t, js::frontend::ParserAnyCharsAccess<js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t> > >::getTokenInternal js/src/frontend/TokenStream.cpp
8 xul.dll js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::objectLiteral js/src/frontend/Parser.cpp:9547
9 xul.dll js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::primaryExpr js/src/frontend/Parser.cpp:9970

Comment 1

[Andrew Overholt [:overholt]]

Jan and Boris have been doing some work in this area so they may have salient thoughts.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this is a near-null 32-bit crash, at address 0x110.

In a 64-bit build (I don't have a 32-bit one offhand), the offset of principals_ in Realm is 0x1d0, so it seems plausible that realm is null.

Now why its null, I have no idea. I'd expect ScriptLoader::EvaluateScript (frame 72 of the stack) to put the JSContext in a Realm, and then nothing in that stack should take it out of there....

Waldo, any idea given all the tokenizer/parser code on there?

Comment 3

[Jan de Mooij [:jandem]]

Looking at the crash reports, not all of them are in the frontend, for example this one:

https://crash-stats.mozilla.com/report/index/aebd2d92-2bfe-47e2-9726-d14000190201

However almost all of them are under js::ReportOutOfMemory => mozilla::CycleCollectedJSRuntime::ErrorInterceptor::interceptError.

The ErrorInterceptor thing is Nightly-only.

I wonder if the JSContext* we get from CycleCollectedJSContext is corrupt or doesn't match "our" context. In debug builds we assert they're equal:

https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/base/nsContentUtils.cpp#2210

It's pretty weird because CycleCollectedJSContext's context is used in a lot of places...

Comment 4

[Jeff Walden [:Waldo]]

Maybe a diagnostic patch that crashes if they're unequal might help sort things out?

This doesn't look parsing/tokenizing related to me from these stacks, much likelier some sort of embedding-provided thing. Gecko people would have better ideas what might be up than me, particularly if the assert-equal thing in comment 3 doesn't pan out.

Comment 5

[Daniel Bodea [:danibodea]]

Considering the "regressionwindow-wanted" tag, I could try to find the regression if some steps to reproduce are provided.
Please NI me if a working STR is obtained. Thanks.

Comment 6

[Marcia Knous [:marcia]]

This crash has 12 crashes/11 installs in the last 7 days on 68 nightly. I don't see any comments or anything that would lead us to a set of STR.

Comment 7

[Jason Orendorff [:jorendorff]]

OK, so this is maybe barely actionable? If we change this assertion to a MOZ_RELEASE_ASSERT, likely outcomes are:

• crash volume of the current signature goes to 0: we start crashing at the assertion instead. In this case we have taken one step up the causal chain, but it is probably then a dead end.

• nothing changes. In this case it's a dead end.

Does not sound like an especially great use of time => P3.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.