Closed Bug 1631340 Opened 3 years ago Closed 8 months ago

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

Categories

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

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
99 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox75 --- unaffected
firefox76 --- unaffected
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 --- wontfix
firefox96 --- wontfix
firefox97 --- wontfix
firefox98 --- fixed
firefox99 --- fixed

People

(Reporter: gsvelto, Assigned: jjalkanen)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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

Flags: needinfo?(jvarga)
Priority: -- → P2

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

Regressed by: 1620152
Has Regression Range: --- → yes

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

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

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

(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.

Flags: needinfo?(jvarga) → needinfo?(jjalkanen)
Assignee: nobody → jjalkanen
Flags: needinfo?(jjalkanen)

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.

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

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.

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

Flags: needinfo?(jstutte)

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

Flags: needinfo?(jstutte)
Pushed by jjalkanen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2c547fafc69
Return empty buffer on failed Snappy uncompress. r=dom-storage-reviewers,asuth

(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?

Flags: needinfo?(jjalkanen)
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch

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

(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.

Flags: needinfo?(jjalkanen)

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.

Flags: needinfo?(jjalkanen)

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.

Flags: needinfo?(jjalkanen)

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.

Flags: needinfo?(jjalkanen)

(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 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
Flags: needinfo?(jjalkanen)
Attachment #9258304 - Flags: approval-mozilla-beta?

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!

Attachment #9258304 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.