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

VERIFIED FIXED in Firefox 61

Status

()

defect
P2
major
VERIFIED FIXED
a year ago
6 months ago

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

a year ago
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

a year ago
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

a year ago
Oh, crud. That sounds all too plausible.
(Assignee)

Comment 4

a year 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)

Updated

a year ago
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

a year ago
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

a year ago
Blocks: 1458640
Duplicate of this bug: 1458515

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04e165cb504f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
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

a year ago
Yes, I can confirm that perf-html.io is working again, with the "broken" profile.
Flags: needinfo?(mstange)
Status: RESOLVED → VERIFIED

Comment 16

10 months 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).
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

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