1646330 - Crash in [@ mozilla::SharedStyleSheetCache::InsertIntoCompleteCacheIfNeeded]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-2f5c6786-7faf-40de-8f8d-43ea20200616.

Top 10 frames of crashing thread:

0 xul.dll mozilla::SharedStyleSheetCache::InsertIntoCompleteCacheIfNeeded layout/style/SharedStyleSheetCache.cpp:397
1 xul.dll static mozilla::SharedStyleSheetCache::LoadCompletedInternal layout/style/SharedStyleSheetCache.cpp:327
2 xul.dll static mozilla::SharedStyleSheetCache::LoadCompleted layout/style/SharedStyleSheetCache.cpp:263
3 xul.dll mozilla::MozPromise<bool, bool, 1>::ThenValue<`lambda at /builds/worker/checkouts/gecko/layout/style/Loader.cpp:1424:11', `lambda at /builds/worker/checkouts/gecko/layout/style/Loader.cpp:1429:11'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:769
4 xul.dll mozilla::MozPromise<bool, bool, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:410
5 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:146
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:501
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308

There are 5 crashes (from 5 installations) in nightly 79 starting with buildid 20200615214838. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1599160.

[1] https://hg.mozilla.org/mozilla-central/rev?node=c5b351d19a30

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Hmm, there are a couple of URLs, but I haven't reproduced the issue in any of them... Jason, do you know if any fuzzer has hit this yet?

Comment 2

[Jason Kratzer [:jkratzer]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Hmm, there are a couple of URLs, but I haven't reproduced the issue in any of them... Jason, do you know if any fuzzer has hit this yet?

No, unfortunately not. I'll keep an eye out for this bug if it comes in.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

S3 for now, since it's very low crash volume so far. But it's due to the code in question hasn't reached to betas/releases. (that said, Jason and Emilo keep eyes on this, so hopefully this will be fixed before the next beta)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

This is a diagnostic assert, so won't cause any badness in release. Given that and the low volume I think I won't spend a lot of time on it without a test-case or STR. Though I am curious about what could cause this.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Wouldn't domino or any of the other fuzzers have hit this by any chance right?

Comment 7

[Jason Kratzer [:jkratzer]]

No, unfortunately not. We have solid coverage of the code in question but I don't have a record of any crash matching that signature.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

I experienced this bug yesterday when I was looking at the pre-order of the PS5 https://www.amazon.fr/Playstation-Sony-PlayStation-5/dp/B08H93ZRK9 (I am not saying that to make Jason happy :)

Comment 9

[Sean Voisen (:svoisen)]

Bumping up severity based on volume.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1646330 - Downgrade an assertion to a regular MOZ_ASSERT. r=#style

My two guesses about how this can happen are:

• PR_Now() is not guaranteed to be monotonic. We can hit an expired
  entry and in-flight become "unexpired".

• nsLoadGroup flags being mutated mid-load (by stuff like devtools).

I tried to reproduce this in both ways but I couldn't, but it doesn't
seem far-fetched.

In any case this failing this assert is not a correctness issue (this
branch is intended to catch cases where we do too much work, but the
consequence of it being violated is just we have performed an extra
load), so let's not crash nightly / early-beta users as we don't have
any way to reproduce it.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

If we happen to have a repro now I'd be extremely interested anyhow :-)

Comment 12

[Jason Kratzer [:jkratzer]]

No, sorry - still nothing from the fuzzers.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4533f1eb2e43
Downgrade an assertion to a regular MOZ_ASSERT. r=jwatt

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4533f1eb2e43