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

Status: VERIFIED FIXED

(Core :: Storage: IndexedDB, defect, P2)

Description

[Markus Stange [:mstange]]

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

Comment 1

[Markus Stange [:mstange]]

Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3ee25bda924ea97cbda2d8602ede83b30c61eb19&tochange=7b6f6fcaaa177324d1827f9be431e23e9e0573ad

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 3

[Steve Fink [:sfink] [:s:]]

Oh, crud. That sounds all too plausible.

Comment 4

[Steve Fink [:sfink] [:s:]]

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?)

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Ignore Indexed DB stored scopes, as they can now be multiple incorrect values and we no longer need them for anything

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Don't suppose it'd be possible to add a test for this?

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(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?

Comment 8

[Jason Orendorff [:jorendorff]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/04e165cb504f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 14

[Martin Giger [:freaktechnik]]

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

Comment 15

[Markus Stange [:mstange]]

Yes, I can confirm that perf-html.io is working again, with the "broken" profile.

Comment 16

[Ed Lee :Mardak]

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

[Johann Hofmann [:johannh]]

(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

[Jan Varga [:janv]]

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