"bad serialized structured data (incompatible structured clone scope)" on perf-html.io

VERIFIED FIXED in Firefox 61

Status

()

defect
P2
major
VERIFIED FIXED
Last year
Last month

People

(Reporter: mstange, Assigned: sfink)

Tracking

({regression})

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61blocking verified)

Details

Attachments

(1 attachment)

Reporter

Description

Last year
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
Reporter

Updated

Last year
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.
Assignee

Comment 3

Last year
Oh, crud. That sounds all too plausible.
Assignee

Comment 4

Last year
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

Updated

Last year
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+

Comment 9

Last year
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

Updated

Last year
Blocks: 1458640
Duplicate of this bug: 1458515

Comment 11

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/04e165cb504f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Duplicate of this bug: 1458354
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).
Reporter

Comment 15

Last year
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).
See Also: → 1471375
(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

10 months 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)
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.