Closed
Bug 1458320
Opened 7 years ago
Closed 7 years ago
"bad serialized structured data (incompatible structured clone scope)" on perf-html.io
Categories
(Core :: Storage: IndexedDB, defect, P2)
Core
Storage: IndexedDB
Tracking
()
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | blocking | verified |
People
(Reporter: mstange, Assigned: sfink)
References
Details
(Keywords: regression)
Attachments
(1 file)
It looks like today's Firefox Nightly has trouble reading existing data from IndexedDB.
Steps to reproduce, probably (haven't tested it yet):
1. Create a new Firefox profile using yesterday's FirefoxNightly.
2. Start yesterday's FirefoxNightly with that profile, install the Gecko profiler add-on from perf-html.io , capture a profile. (This will put a symbol table into IndexedDB on perf-html.io .)
3. Close that instance of Firefox.
4. Start today's FirefoxNightly with the same Firefox profile.
5. Attempt to grab a profile.
Expected results:
Symbolication should complete properly.
Actual results:
There is an error in the web console:
> bad serialized structured data (incompatible structured clone scope)
It points to this line: https://github.com/devtools-html/perf.html/blob/a60e54d082e285ed6760ff97b1b756c3f3cb8052/src/profile-logic/symbol-store-db.js#L213
Updated•7 years ago
|
Priority: -- → P2
Reporter | ||
Updated•7 years ago
|
Keywords: regression
Comment 2•7 years ago
|
||
It seems like the issue is that for a few days since bug 1434308 landed, we've been writing JS::StructuredCloneScope::DifferentProcess to disk. But bug 1456604 only handles the pre-bug 1434308 implementation that wrote JS::StructuredCloneScope::SameProcessSameThread.
Specifically, this is the hunk that makes IDB okay, which isn't the case for the bad scenario:
+ if (storedScope == JS::StructuredCloneScope::SameProcessSameThread &&
+ allowedScope == JS::StructuredCloneScope::DifferentProcessForIndexedDB)
+ {
+ // Bug 1434308 - allow stored IndexedDB clones to contain what is
+ // essentially DifferentProcess data, but labeled as
+ // SameProcessSameThread (because that's what old code wrote.)
+ allowedScope = JS::StructuredCloneScope::DifferentProcess;
+ return true;
+ }
So we fall through to:
+ if (storedScope < allowedScope) {
+ JS_ReportErrorNumberASCII(context(), GetErrorMessage, nullptr,
+ JSMSG_SC_BAD_SERIALIZED_DATA,
+ "incompatible structured clone scope");
+ return false;
+ }
Which is what errors, because DifferentProcess is indeed < DifferentProcessForIndexedDB.
It seems like at this point the only solution is to further widen the IDB carve-out.
Assignee | ||
Comment 3•7 years ago
|
||
Oh, crud. That sounds all too plausible.
Assignee | ||
Comment 4•7 years ago
|
||
At this point, it seems like DifferentProcessForIndexedDB should probably just allow anything. It'll only be used for IndexedDB, and in that case, the stored scope really isn't very interesting no matter what it is. In a way, that feels cleaner than the more specific cutout I botched. It seems ok to make it be "the scope stored by IndexedDB is wrong and should be ignored". (Rationalize much?)
Flags: needinfo?(sphink)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8972462 -
Flags: review?(jorendorff)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment 6•7 years ago
|
||
Don't suppose it'd be possible to add a test for this?
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
tracking-firefox61:
--- → +
Flags: needinfo?(sphink)
Comment 7•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Don't suppose it'd be possible to add a test for this?
We can add a test for this specific thing, but we'd need a new type of test setup to automatically test this type of regression going forward. Specifically, we'd need a "profile continuity" suite that runs on every nightly (or similar granularity) and checks the result into some type of revision control. Then "check" runs could run the suite without persisting their results.
Elaborating, the IDB disk protocol went from:
SameProcessSameThread ====> DifferentProcess ====> DifferentProcessForIndexedDB
^^^ ^^^ ^^^ No db's checked in for this either.
||| |||
||| No db's checked in for this.
|||
We have tests for this with IDB databases checked in.
:janv, any chance you can easily build on your exist test infra to get us database zips and coverage for the new structured clone types?
Flags: needinfo?(jvarga)
Comment 8•7 years ago
|
||
Comment on attachment 8972462 [details] [diff] [review]
Ignore Indexed DB stored scopes, as they can now be multiple incorrect values and we no longer need them for anything
Review of attachment 8972462 [details] [diff] [review]:
-----------------------------------------------------------------
r=me to the StructuredClone.cpp changes. sfink says the autospider.py change is a mistake.
Attachment #8972462 -
Flags: review?(jorendorff) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04e165cb504f
Ignore Indexed DB stored scopes, as they can now be multiple incorrect values and we no longer need them for anything, r=jorendorff
Comment 11•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 13•7 years ago
|
||
Markus, can you confirm that perf-html.io is working again for you? Google Drive (from the duped bug) looks good to me now, but some additional verification is always welcome :)
Flags: needinfo?(mstange)
Comment 14•7 years ago
|
||
I can confirm that extensions where the indexedDBs were broken in earlier nightlies are now fixed (my bug for that got duped into this one).
Reporter | ||
Comment 15•7 years ago
|
||
Yes, I can confirm that perf-html.io is working again, with the "broken" profile.
Flags: needinfo?(mstange)
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 16•6 years ago
|
||
Has anyone else noticed an increase in problems that might be from IndexedDB failures with 61 going to release? Activity Stream is currently investigating bug 1471375, and we seem to be getting a lot of IndexedDB failures in our telemetry (but unclear if it's related to the bug).
Comment 17•6 years ago
|
||
(In reply to Ed Lee :Mardak from comment #16)
> Has anyone else noticed an increase in problems that might be from IndexedDB
> failures with 61 going to release? Activity Stream is currently
> investigating bug 1471375, and we seem to be getting a lot of IndexedDB
> failures in our telemetry (but unclear if it's related to the bug).
I've just seen bug 1451794 comment 1 so far, in that case it seems related to quota manager?
Comment 18•6 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #7)
> We can add a test for this specific thing, but we'd need a new type of test
> setup to automatically test this type of regression going forward.
> Specifically, we'd need a "profile continuity" suite that runs on every
> nightly (or similar granularity) and checks the result into some type of
> revision control. Then "check" runs could run the suite without persisting
> their results.
Yeah, I will have to figure out how to do that. It would definitely be great to have such check.
Flags: needinfo?(jvarga)
Updated•6 years ago
|
Flags: needinfo?(sphink)
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•