STR:

1. open https://www.kuehhorn.de/heizungsrechner

Expected:

No crash.

Actual:

The tab crashes [@ js::InternalCallOrConstruct ]:
https://crash-stats.mozilla.org/report/index/1cb32f8a-435b-45bc-9b4c-543740210701

This only happens in Firefox 91 Nightly (tested in a new profile) but not in Firefox 90 Beta or the stable release of Firefox 89.

Regression range:

 7:14.22 INFO: Narrowed nightly regression window from [2020-05-21, 2020-05-23] (2 days) to [2020-05-21, 2020-05-22] (1 days) (~0 steps left)
 7:14.22 INFO: Got as far as we can go bisecting nightlies...
 7:14.22 INFO: Last good revision: 92c11f0bf14b71b70bec5351212ae237707f4a62 (2020-05-21)
 7:14.22 INFO: First bad revision: d6abd35b54ad39fa030217716b172fa9883bf3c8 (2020-05-22)
 7:14.22 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=92c11f0bf14b71b70bec5351212ae237707f4a62&tochange=d6abd35b54ad39fa030217716b172fa9883bf3c8

Bug 1362154 is in the regression range. Running the website in a debug build asserts with IsObjectValueInCompartment(v, compartment()) from:

#0  0x00007ffff6db338d in nanosleep () at ../sysdeps/unix/syscall-template.S:84
#1  0x00007ffff6db32da in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55
#2  0x00007fffe993de77 in common_crap_handler(int, void const*) () from /tmp/firefox/libxul.so
#3  0x00007fffe993df88 in child_ah_crap_handler(int) () from /tmp/firefox/libxul.so
#4  0x00007fffeaa60735 in WasmTrapHandler(int, siginfo_t*, void*) () from /tmp/firefox/libxul.so
#5  <signal handler called>
#6  0x00007fffe9a45ca8 in js::NativeObject::checkStoredValue(JS::Value const&) () from /tmp/firefox/libxul.so
#7  0x00007fffe9a51abe in js::NativeObject::setSlot(unsigned int, JS::Value const&) () from /tmp/firefox/libxul.so
#8  0x00007fffe9cc3e68 in js::CreateRegExpMatchResult(JSContext*, JS::Handle<js::RegExpShared*>, JS::Handle<JSString*>, js::MatchPairs const&, JS::MutableHandle<JS::Value>) () from /tmp/firefox/libxul.so
#9  0x00007fffe9ccc72a in RegExpMatcherImpl(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSString*>, int, JS::MutableHandle<JS::Value>) () from /tmp/firefox/libxul.so
#10 0x00007fffe9ccc1c5 in js::RegExpMatcher(JSContext*, unsigned int, JS::Value*) () from /tmp/firefox/libxul.so
#11 0x00007fffe9a33fc1 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) () from /tmp/firefox/libxul.so
#12 0x00007fffe9a336f6 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) () from /tmp/firefox/libxul.so
#13 0x00007fffe9a34ef1 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) () from /tmp/firefox/libxul.so
#14 0x00007fffe9a28a8d in Interpret(JSContext*, js::RunState&) () from /tmp/firefox/libxul.so
[...]

That assertion can be triggered with:

g = newGlobal({sameZoneAs: this});

re1 = RegExp("(?<a>a)")
re2 = g.RegExp("(?<a>a)")

re1.exec("a");
re2.exec("a");

RegExpShared is shared by all compartments in the same zone, but reusing the named capturing object template across different compartments isn't sound.

Thanks for the patch. I've assigned myself as reviewer, because Phabricator apparently didn't parse the "r=iain", but I don't appear to be able to stamp it until you submit it for review. (Unless you have additional planned changes?)

(In case we were looking for a second STR, I just hit this trying to buy concert tickets on ticketmaster.ca right at checkout :)

This is quickly reaching the point where we should consider uplifting this change. :-)

Is anything blocking landing this?

There was an open question about a possible performance regression. It got answered yesterday, but Phabricator didn't send me an email, so I didn't see Iain's answer.

https://hg.mozilla.org/mozilla-central/rev/38aa248ef576

The patch landed in nightly and beta is affected.
:anba, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

We had only 3 crashes in 91 beta so far, I think this can ride the 92 train.

(In reply to Pascal Chevrel:pascalc from comment #13)

We had only 3 crashes in 91 beta so far, I think this can ride the 92 train.

Hmm, someone didn't report their crash reports. ;-) We should have at least four reports for the sites:

• https://www.kuehhorn.de
• https://www.ticketmaster.ca
• https://shop.maoup.com.tw
• https://www.economist.com

I still think uplifting makes sense, I was just waiting a couple of days to make sure we don't see any regressions on Nightly.

(In reply to André Bargull [:anba] from comment #14)

I still think uplifting makes sense, I was just waiting a couple of days to make sure we don't see any regressions on Nightly.

I agree; IMO if this is not uplifted, we're going to wind up with very obvious crashing
on release. For the instance I found (bug 1720451), it's not like I spent any time looking
for a test failure. I just clicked on a link (recommended by Pocket) and it crashed.

Why do we have almost no crashes on beta at the moment? We have a lot more beta users than nightly users so that seems strange to me that this is a problem on nightly and not beta at the moment.

The bug should only occur in the browser when this line is taken. Unfortunately I can't tell when the browser does that or if there's some preference which makes it more or less likely that only Nightly builds are affected.

Also keep in mind that this is related to a relatively new RegExp feature (named groups) so crash volume will increase over time as more websites start using that in their code.

Whoops, I didn't check the condition where the crash happens:

This is only crashing on Nightly, because the crash happens here and diagnostic assertions are only enabled on Nightly and debug builds. So on Beta and Release, we need to find some other place where an unwrapped cross-compartment object can cause issues.

As another datapoint, a build of m-beta (653762:8f2a5f1e3207), using the
mozconfig shown below, does not crash on x86_64-linux when loading the
following 3 test cases:

https://www.kuehhorn.de/heizungsrechner
https://shop.maoup.com.tw/collections/%E7%8B%97%E7%8B%97%E6%B4%97%E6%AF%9B%E7%B2%BE
https://www.economist.com/leaders/2021/07/03/the-real-risk-to-americas-democracy

. $topsrcdir/browser/config/mozconfig
export CC="clang -Og -gline-tables-only"
export CXX="clang++ -Og -gline-tables-only"
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/clang-Og-nondebug-systemalloc
ac_add_options --enable-tests
ac_add_options --enable-optimize="-Og -gline-tables-only"
ac_add_options --enable-debug-symbols
ac_add_options --disable-debug
ac_add_options --enable-valgrind
ac_add_options --disable-jemalloc
ac_add_options --enable-profiling
ac_add_options --disable-crashreporter

We're nearly out of time to land a fix for 91 this cycle. If we're going to uplift it, we should get the request up ASAP.

Comment on attachment 9229602 [details]
Bug 1718842: Use correct compartment and realm for group objects. r=iain!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes when diagnostic assertions are enabled. When diagnostic assertions aren't enabled, an object from a different compartment is passed around without a cross-compartment wrapper. As this should never happen, the JavaScript engine isn't prepared for this case and it's unclear what can go wrong.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change should be unrisky, because it uses well tested functions (js::Shape and shape caches are the cornerstones of the JS engine).
• String changes made/needed:

Comment on attachment 9229602 [details]
Bug 1718842: Use correct compartment and realm for group objects. r=iain!

Approved for uplift on the beta branch before Monday's beta to release merge.

https://hg.mozilla.org/releases/mozilla-beta/rev/25c0f8ea51f6