Crash in [@ mozilla::dom::LSValue::Converter::Converter]
Categories
(Core :: Storage: localStorage & sessionStorage, defect, P2)
Tracking
()
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)
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
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
Updated•5 years ago
|
Comment 1•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 2•5 years ago
|
||
SnappyUncompress(aValue.mBuffer, buffer);
is failing and LSValue::Converter
is ignoring the failure, returning an empty buffer. Is that OK?
Comment 3•4 years ago
|
||
fix-optional for 77 as we are not seeing crashes for this signature in our first beta.
Updated•4 years ago
|
Updated•4 years ago
|
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•3 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #2)
SnappyUncompress(aValue.mBuffer, buffer);
is failing andLSValue::Converter
is ignoring the failure, returning an empty buffer. Is that OK?
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.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
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.
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 7•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
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•3 years ago
|
||
Jens, can we get this reviewed? Looks like it could be a good uplift candidate for 97 still.
Comment 10•3 years ago
•
|
||
We do not think this is worth uplifting (and are busy with bug 1752265, too)
Comment 11•3 years ago
|
||
Comment 12•3 years ago
|
||
(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•3 years ago
|
||
bugherder |
Comment 14•3 years ago
|
||
Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 15•3 years ago
|
||
(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.
Updated•3 years ago
|
Comment 16•3 years ago
|
||
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.
Assignee | ||
Comment 17•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
(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
Assignee | ||
Comment 20•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
bugherder uplift |
Description
•