1286914 - Prevent creation of origin directories which are not supported by our origin parser on nightly

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

With my regular Firefox profile, a web page's attempt to open an IndexedDB database fails with UnknownError (which happens to cause the page to assume the browser is in private browsing mode).

A debug build (from June 15, as newer debug builds are crashing for me on startup, although the bug manifests in the latest optimized build) reports:

>[Parent 6191] WARNING: '!result', file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/quota/ActorsParent.cpp, line 6418
>[Parent 6191] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/quota/ActorsParent.cpp, line 7383
>[Parent 6191] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/quota/ActorsParent.cpp, line 4110
>[Parent 6191] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/quota/ActorsParent.cpp, line 4254
>[Parent 6191] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/quota/ActorsParent.cpp, line 4396
>[Parent 6191] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp, line 20765
>[Parent 6191] WARNING: 'NS_FAILED(rv)', file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp, line 20610
>[Parent 6191] WARNING: Converting non-IndexedDB error code (0x80004005) to NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR: file /builds/slave/m-cen-m64-d-000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp, line 579

cc: asuth in the hope that he'll be able to help diagnose, given bug 1143003, comment 41.

Comment 1

[Jan Varga [:janv]]

This looks like we are not able to parse origin directory name. There should be an error displayed in the browser console.
Something like: "Origin '%s' failed to parse, handled tokens: %s"

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jan Varga [:janv] from comment #1)
> This looks like we are not able to parse origin directory name. There should
> be an error displayed in the browser console.
> Something like: "Origin '%s' failed to parse, handled tokens: %s"

Indeed, loading the page generates this message in the Browser Console:

13:44:22.975 Quota Origin 'about+reader+url=https%3A%2F%2Fmichielbdejong.github.io%2Fmichielbdejong.com%2Fblog%2F16.html' failed to parse, handled tokens: 'about', 'reader': ActorsParent.cpp:6599(unknown)

Comment 3

[Myk Melez [:myk] [@mykmelez]]

It looks like this happens on all pages, not just the one where I first noticed it.  I can trigger it on any page by evaluating this simple statement in the Web Console:

> window.indexedDB.open('foo').onerror = function() { console.log(this.error.name) };

If I evaluate it in the Browser Console, I get some additional messages:

>Quota 'url=https%3A%2F%2Fmichielbdejong.github.io%2Fmichielbdejong.com%2Fblog%2F16.html' is not a valid port number!: ActorsParent.cpp:6848(unknown)
>Quota Origin 'about+reader+url=https%3A%2F%2Fmichielbdejong.github.io%2Fmichielbdejong.com%2Fblog%2F16.html' failed to parse, handled tokens: 'about', 'reader': ActorsParent.cpp:6599(unknown)
>IndexedDB UnknownErr: ActorsParent.cpp:585(unknown)
>UnknownErrorindexed-db.js:59:9

Comment 4

[Jan Varga [:janv]]

Yeah, the parser doesn't support this kind of URL, it needs to be extended to support it.
I'll take a look.
Thanks for the info.

Comment 5

[Andrew Overholt [:overholt]]

Is this a regression?

Comment 6

[Jan Varga [:janv]]

Well, we didn't change anything in the parser. It's just a new type of origin string that is used for storing data.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Presumably this string was generated by Firefox Desktop's about:reader feature, which I've used on only a couple of occasions.  So other users who have used that feature may be experiencing the same problem, even if they don't identify it as an IndexedDB failure.

Comment 8

[Jan Varga [:janv]]

But this only happens when the .metadata file for the origin directory is lost and needs to be recreated.
And also when we sometimes upgrade whole <profile>/storage directory structure.
Anyway, it definitely needs to be fixed.

Comment 9

[Jan Varga [:janv]]

Myk, could you please zip/package the origin directory and attach it here ?
I somehow can't recreate it locally. Thanks

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jan Varga [:janv] from comment #9)
> Myk, could you please zip/package the origin directory and attach it here ?
> I somehow can't recreate it locally. Thanks

Jan, I sent it to you privately, as I'm unfamiliar with the feature, so I'm unsure whether it's possible to leak personal info by posting it publicly.

Comment 11

[Jan Varga [:janv]]

It's the DOM cache which stores something for that origin. Not sure if we should allow storing something for these origins at all, or that the origin string contains a query string. I'll file a new bug for that later. We need to fix the parser either way.
I'm testing a fix right now.

Comment 12

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

I think for about URL's we want the origin to just be "about:WHAT".  I've added "about:foo" things before, and the rationale was usually just to make a short URL that the user can type.  (I've also added custom protocols for when I wanted payloads, like in this case.)  I expect that's the primary goal of most about: implementations, and the design choices are a cross between cargo culting and just trying to get something/anything to work.  If a fancier use case is desired, a big discussion should happen on dev-platform.

Comment 13

[Jan Varga [:janv]]

Exactly, I just got an idea, we should add a debug only check to test origin directories before they are created, especially if they can be parsed back with the origin parser.
That should catch similar bugs much earlier.

Comment 15

[Jan Varga [:janv]]

Attachment: Part 1: Prevent creation of origin directories which are not supported by our origin parser

So I pushed this to try and found out that some tests fail because the moz-extension schema is not supported.
This check will also run in optimized builds, but only in Nightly and Aurora. It should catch any other unsupported origins in future.

Comment 16

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

Comment on attachment 8821015 [details] [diff] [review]
Part 1: Prevent creation of origin directories which are not supported by our origin parser

Review of attachment 8821015 [details] [diff] [review]:
-----------------------------------------------------------------

Restating my understanding, this patch adds a nightly/alpha-only verification step in QuotaManager::EnsureOriginIsInitialized to round-trip any to-be-created origin's directory name through the origin parser to make sure it's supported and fail if it is not.  This will not affect release or beta builds, presumably because of concerns over the additional resulting fstat() calls and maybe CPU concerns?  The expectation is that any new URI scheme introduced which needs quota-manager-backed storage will be in-tree and generate test failures (as well as the developer hopefully noticing the failures and that the quota manager logs errors to the browser console).

The lack of the check on release/beta isn't a meaningful concern because:
- Custom URI/scheme implementations are already ill-advised and not really supported.
- The pref that disables extension signing doesn't work on release/beta either, so extension developers are very likely to be on aurora or nightly channels 

An equally not-on-release/beta test is added that tries to create an IndexedDB database for an FTP url and verifies failure.  This is future-proof because we'll never want to support storage under origins for ftp.

Comment 17

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #16)

Thanks for quick review!

> sure it's supported and fail if it is not.  This will not affect release or
> beta builds, presumably because of concerns over the additional resulting
> fstat() calls and maybe CPU concerns?

Yes, it's non trivial overhead I think.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b63119e4fc
Part 1: Prevent creation of origin directories which are not supported by our origin parser; r=asuth

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c8b63119e4fc

Comment 20

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

In the interest in cleaning up the set of open bugs relating to mysterious QuotaManager/IndexedDB breakage a little, I'm marking this fixed and renaming to describe the action taken that landed.

Comment 21

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.