1631340 - Crash in [@ mozilla::dom::LSValue::Converter::Converter]

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, defect, P2)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

This bug is for crash report bp-67546159-e786-4bd8-bd55-e78e10200418.

Top 10 frames of crashing thread:

0 XUL mozilla::dom::LSValue::Converter::Converter dom/localstorage/LSValue.h:90
1 XUL mozilla::dom::LSSnapshot::Init dom/localstorage/LSValue.h:85
2 XUL mozilla::dom::LSDatabase::EnsureSnapshot dom/localstorage/LSDatabase.cpp:325
3 XUL mozilla::dom::LSObject::GetItem dom/localstorage/LSObject.cpp:592
4 XUL mozilla::dom::Storage_Binding::getItem dom/bindings/StorageBinding.cpp:161
5 XUL bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3205
6 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:584
7 XUL Interpret js/src/vm/Interpreter.cpp:647
8 XUL js::ExecuteKernel js/src/vm/Interpreter.cpp:840
9 XUL js::Execute js/src/vm/Interpreter.cpp:873

macOS-specific crash, seems to have started with buildid 20200411093441. There's two useful comments in the reports:

plex homepage launching

and

Trying to launch Plex web sign in. disabling extensions and removing cache and cookies don't fix the issue. Occurs after sign in and attempting to launch the web page.

Plenty of the crashing URLs point to: https://app.plex.tv/desktop

Comment 1

[Ryan Hunt [:rhunt] (on leave until early May)]

I've been getting this for a bit, assumed it was something in my setup. It happens on any Github page for me.

I did mozregression and found the following range [1]. It looks like this assertion was made to crash on release builds recently.

[1] https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2dd2598b3edbe623eb91e15a0280223c74d82470&tochange=9b00ab9d1a8d7041b45e7bf08c9663e2a8cf5b03

Comment 2

[Chris Peterson [:cpeterson]]

SnappyUncompress(aValue.mBuffer, buffer); is failing and LSValue::Converter is ignoring the failure, returning an empty buffer. Is that OK?

https://hg.mozilla.org/mozilla-central/annotate/853b0e791775ce726149209092a003ed5f001b0c/dom/localstorage/LSValue.h#l90

Comment 3

[Pascal Chevrel:pascalc (PTO until April 26)]

fix-optional for 77 as we are not seeing crashes for this signature in our first beta.

Comment 4

[u608768]

We're still seeing a number of these every few days in Nightly and Beta. Appears to be the same device each time.

Comment 5

[Jan Varga [:janv]]

(In reply to Chris Peterson [:cpeterson] from comment #2)

SnappyUncompress(aValue.mBuffer, buffer); is failing and LSValue::Converter is ignoring the failure, returning an empty buffer. Is that OK?

https://hg.mozilla.org/mozilla-central/annotate/853b0e791775ce726149209092a003ed5f001b0c/dom/localstorage/LSValue.h#l90

Jari, SnappyUncompress can fail due to multiple reasons.
Since you worked in this area recently, can you investigate how we can fix/mitigate this ?
Thanks.

Note, MOZ_ALWAYS_TRUE is like MOZ_DIAGNOSTIC_ASSERT, so it doesn't crash the app in release builds, so we don't know how often this is happening in release builds.

Comment 6

[Jari Jalkanen]

If I recall correctly, current version of snappy decodes directly into the output array. I think it might be safer to return an empty buffer because we would then end up in a well-understood state after the decompression error.

Comment 7

[Jari Jalkanen]

Attachment: Bug 1631340 - Return empty buffer on failed Snappy uncompress. r=#dom-storage-reviewers

Comment 8

[Jari Jalkanen]

The uncompressed buffer is in the database and may be modified so that the decompression fails, or perhaps the memory allocation fails. The proposed fix in these cases is to emit a warning and return an empty buffer.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Jens, can we get this reviewed? Looks like it could be a good uplift candidate for 97 still.

Comment 10

[Jens Stutte [:jstutte]]

We do not think this is worth uplifting (and are busy with bug 1752265, too)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2c547fafc69
Return empty buffer on failed Snappy uncompress. r=dom-storage-reviewers,asuth

Comment 12

[Jens Stutte [:jstutte]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Jens, can we get this reviewed? Looks like it could be a good uplift candidate for 97 still.

We could consider to uplift this to 98, being in the very beginning of the beta cycle. Jari, do you think it is worth it?

Comment 13

[Cosmin Sabou [:CosminS]]

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

Comment 14

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 15

[Jari Jalkanen]

(In reply to Jens Stutte [:jstutte] from comment #12)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Jens, can we get this reviewed? Looks like it could be a good uplift candidate for 97 still.

We could consider to uplift this to 98, being in the very beginning of the beta cycle. Jari, do you think it is worth it?

We don't know how often there is an issue in the release builds. In debug builds, this is not common.

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

There are dozens of crashes for 98 beta 2 and 3 but none on the release channel, if you consider that this is a safe patch, we can definitely uplift to 98 and make it more stable to our beta users.

Comment 17

[Jari Jalkanen]

This can be considered as a safe patch and for quality purposes, it would also be more interesting to get an error here instead of a crash.

Comment 18

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

The patch landed in nightly and beta is affected.
:jjalkanen, 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.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Jari Jalkanen from comment #17)

This can be considered as a safe patch and for quality purposes, it would also be more interesting to get an error here instead of a crash.

Jari could you full the uplift request? Thanks
https://video.chevrel.org/demos/upliftRequestForm.mp4

Comment 20

[Jari Jalkanen]

Comment on attachment 9258304 [details]
Bug 1631340 - Return empty buffer on failed Snappy uncompress. r=#dom-storage-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Without the patch, the debug build will crash when the profile data is corrupted and cannot be decompressed, otherwise the expected behavior is the same as before, in the release build.
• 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 errors were already handled with an assert, now the behaviour in the debug build is essentially the same as in the release build.
• String changes made/needed: None

Comment 21

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9258304 [details]
Bug 1631340 - Return empty buffer on failed Snappy uncompress. r=#dom-storage-reviewers

Thanks, uplift approved for 98 beta 5!

Comment 22

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/31ab05d53a7e