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)

defect

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
Priority: -- → P2
Keywords: regression
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.
Oh, crud. That sounds all too plausible.
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: nobody → sphink
Status: NEW → ASSIGNED
Don't suppose it'd be possible to add a test for this?
(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 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
Blocks: 1458640
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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)
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).
Yes, I can confirm that perf-html.io is working again, with the "broken" profile.
Flags: needinfo?(mstange)
Status: RESOLVED → VERIFIED
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).
(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?
(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)
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: