1684838 - Crash in [@ mozilla::dom::cache::db::CacheKeys]

Status: RESOLVED FIXED

(Core :: Storage: Cache API, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

New in 85.0 (only observed in betas, could be explained by low volume). The signature was known before but only affected <=1 installation per version.

Crash report: https://crash-stats.mozilla.org/report/index/9f542882-fcd6-4c9f-853b-55d900210104

Common crash sites:

• https://web.whatsapp.com/
• https://www.youtube.com/
• https://www.tagesschau.de/

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::cache::db::CacheKeys dom/cache/DBSchema.cpp:929
1 xul.dll mozilla::dom::cache::Manager::CacheKeysAction::RunSyncWithDBOnTarget dom/cache/Manager.cpp:1208
2 xul.dll mozilla::dom::cache::SyncDBAction::RunWithDBOnTarget dom/cache/DBAction.cpp:183
3 xul.dll mozilla::dom::cache::DBAction::RunOnTarget dom/cache/DBAction.cpp:124
4 xul.dll mozilla::dom::cache::Context::ActionRunnable::Run dom/cache/Context.cpp:647
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1200
6 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:332
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:327
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:309
9 xul.dll static nsThread::ThreadFunc xpcom/threads/nsThread.cpp:441

Comment 1

[Jens Stutte [:jstutte]]

From this report's dump it seems that we fail here with a null.

As savedRequest lives on the stack, the only remaining variable I see that might cause this is state, which somehow comes to live through the CACHE_TRY_INSPECT magic.

Assuming that bug 1678030 regressed this, as the first spike is right after bug 1678030 made it into beta.

Comment 2

[Simon Giesecke [:sg] [he/him]]

This was buggy before Bug 1678030. However, Bug 1678030 turned this into a nullptr crash.

Before Bug 1678030, we were assuming that hasMoreData was true after https://searchfox.org/mozilla-central/rev/1ff0e248eb784915f7838f2afcdf0078d4884630/dom/cache/DBSchema.cpp#2396-2400, but apparently this wasn't always the case. We should propagate this as an error instead.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1684838 - Check result of CreateAndExecuteSingleStepStatement in ReadRequest. r=#dom-workers-and-storage

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8625d790fe8
Check result of CreateAndExecuteSingleStepStatement in ReadRequest. r=jstutte

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/a8625d790fe8

Comment 6

[Julien Cristau [:jcristau]]

Might be worth picking that up if we have a 85 dot release. If you agree can you request uplift to release?

Comment 7

[Simon Giesecke [:sg] [he/him]]

Comment on attachment 9197737 [details]
Bug 1684838 - Check result of CreateAndExecuteSingleStepStatement in ReadRequest. r=#dom-workers-and-storage

Beta/Release Uplift Approval Request

• User impact if declined: Exposure to parent process crashes in case of an unexpected Cache API state. The patch doesn't fix the root cause of this (Bug 1687305 tracks this), but ensures an error is propagated rather than crashing the parent process.
• Is this code covered by automated tests?: No
• 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 essentially restores the behaviour before Bug 1678030.
• String changes made/needed:

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 9197737 [details]
Bug 1684838 - Check result of CreateAndExecuteSingleStepStatement in ReadRequest. r=#dom-workers-and-storage

crash fix, approved for 85.0.1

Comment 9

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/4eb503024f71

Comment 10

[Julien Cristau [:jcristau]]

Adding to the 85.0.1 release notes:
"Fixed a browser crash in case of unexpected Cache API state (bug 1684838)."