1345760 - Correctly handle unsupported schema versions of localStorage

Status: REOPENED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

If a profile was opened by Nightly, the schema version will be bumped to 2, then if we open the same profile again with Firefox 52, it will crash in debug mode.

It should be handled (e.g. prompt a warning or delete the storage), instead of crash in debug mode and silently ignored in release mode.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Sounds like a dup of bug 1246615?

Comment 2

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

I guess so, maybe you can set a block flag or just mark as duplicate.

Comment 4

[Jan Varga [:janv]]

Well we shouldn't crash even in 52. We should get a warning instead:
"Unable to initialize storage, version is too high!"
and then IndexedDB/DOM cache should just error out.

Do you have stack trace for this crash. I can also try to build 52 myself and see what happens.

Comment 5

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

I was crashed at here:
https://dxr.mozilla.org/mozilla-release/rev/dbb35200d46931343fd853cbe1af7688794b0940/dom/storage/DOMStorageDBUpdater.cpp#397

I'm still building on my laptop, once it's done I'll post the whole stack.

Comment 6

[Jan Varga [:janv]]

So, this is actually *Local Storage* bug, not Quota manager.
Local Storage is supposed to be downgrade compatible.
CCing Kris who implemented an upgrade for Local Storage recently.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Jan for jumping in and correcting my guess.

Comment 8

[Jan Varga [:janv]]

Here we go, downgrade-compatible changes :)
I'm afraid that we need to patch Update() in StorageDBUpdater.cpp on all supported branches (aurora, beta, esr).

Comment 9

[Jan Varga [:janv]]

and maybe even current release

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jan Varga [:janv] from comment #8)
> Here we go, downgrade-compatible changes :)

Oh, does that mean it's a regression since 52?

> I'm afraid that we need to patch Update() in StorageDBUpdater.cpp on all
> supported branches (aurora, beta, esr).

Does that mean you suggest we need to prioritize this, that said P1? Thank you.

Comment 11

[Jan Varga [:janv]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> (In reply to Jan Varga [:janv] from comment #8)
> > Here we go, downgrade-compatible changes :)
> 
> Oh, does that mean it's a regression since 52?

No, I think bug 1314361 caused 52 to assert. The assertion ends up as a crash in debug version.

> 
> > I'm afraid that we need to patch Update() in StorageDBUpdater.cpp on all
> > supported branches (aurora, beta, esr).
> 
> Does that mean you suggest we need to prioritize this, that said P1? Thank
> you.

It only crashes in debug version, so I don't it's P1