889442 - Remove special about: pages code from DOM Storage

Status: RESOLVED FIXED

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

Description

[Marco Bonardo [:mak]]

Once bug 729777 is fixed, we want to remove any code that allows DOM Storage to specially handle about pages, so that there's not risk of abuses.

I think this is the only relevant code, IIRC there is no test, but I will probably do a try run just to be sure.
http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/DOMStorageManager.cpp#153

Comment 1

[Marco Bonardo [:mak]]

and while there, we should probably run some migration to remove chromeappsstore.sqlite
I guess a simple migration UI step in browser code would satisfy this requirement, I don't have better ideas on where to put it, since we don't have a "profile maintenance component".

Comment 2

[Marco Bonardo [:mak]]

Attachment: DOM Storage patch

Comment 3

[Marco Bonardo [:mak]]

Attachment: remove chromeappsstore.sqlite

I will now push to try, just in case.

Comment 4

[Marco Bonardo [:mak]]

Attachment: remove chromeappsstore.sqlite v2

I guess I should not try/catch OS.File since it's async!

Comment 5

[Marco Bonardo [:mak]]

Just a couple words regarding chromeappsstore.sqlite removal, we stopped using it in Firefox 20, from that version on we supported migrating newtab page entries from it to the new storage, we are now 5 versions and 30 weeks later, so I feel like it's ok to proceed with the removal.

Comment 6

[Marco Bonardo [:mak]]

https://tbpl.mozilla.org/?tree=Try&rev=0547020e17e4

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 770873 [details] [diff] [review]
remove chromeappsstore.sqlite v2

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

I don't know of any policies of how long to keep legacy code but 5 versions feels okay to me as this is really non-critical data. If we're removing it we can also get rid of the migration code in NewTabUtils.jsm.

Comment 8

[Marco Bonardo [:mak]]

Right, for some reason I was thinking we may still need the migration code, but actually we don't. Will make a separate patch for that.

Comment 9

[Marco Bonardo [:mak]]

Attachment: remove newTab migration

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 770880 [details] [diff] [review]
remove newTab migration

ugh, not ready

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 770880 [details] [diff] [review]
remove newTab migration

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

We can also get rid of the FileUtils.jsm import at the top.
We should remove toolkit/modules/tests/xpcshell/test_newtab-migrate-v1.js.

::: toolkit/modules/NewTabUtils.jsm
@@ +79,5 @@
>      if (this._storedVersion < this._version) {
>        // This is either an upgrade, or version information is missing.
>        if (this._storedVersion < 1) {
> +        // Version 1 moved data from DOM Storage to prefs. No more supported.
> +        throw new Error("Unsupported newTab storage version");

We'll lose data here but I don't think we should throw. I think we should just silently update the version number.

Comment 12

[Marco Bonardo [:mak]]

Attachment: remove newTab migration v2

So, the only problematic case is what to do when version information is missing, that's hard to solve when it's not stored in the same store, luckily prefs are used for both now and we only have one supported version, so it's not that important.
I defaulted setting pref to the first supported version, so new profiles will go through migration steps if we add more migrations, the alternative would have been to default to latest version, but then if versioning info gets lost apart from store, the store may be broken. There's some downsides both ways, but as I said for now we probably don't care much.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 770915 [details] [diff] [review]
remove newTab migration v2

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

::: toolkit/modules/NewTabUtils.jsm
@@ +121,5 @@
> +        // - it's a profile where versioning information got lost
> +        // In this case we still run through all of the valid migrations,
> +        // starting from 1, as if it was a downgrade.  As previously stated the
> +        // migrations should already support running on an updated store.
> +        this.__storedVersion = 1;

I wonder, can't we have the same by just setting the default value of 'browser.newtabpage.storageVersion' to 1? And get rid of all the try-catch stuff?

Comment 14

[Marco Bonardo [:mak]]

Even if I move to a pref, I should still have a comment explaining that it's the first supported version and so on, the comment would end up being detached in all.js since otherwise I should still try catch if we'd stick the pref only in firefox.js.
Personally I'd prefer to keep it in the module, so all comments stick together and the logic is better explained, but I don't have strong feelings about that.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Ok, feel free to ignore my comment then. I don't think this is something worth thinking too much about :)

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/330f0e58a4ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/056f0e496643
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4efb0f8d7e

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for breaking Jetpack tests (see bug 890089):
https://hg.mozilla.org/integration/mozilla-inbound/rev/681e4730cb6b

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like browser-chrome tests got broken too? https://tbpl.mozilla.org/php/getParsedLog.php?id=24911878&tree=Mozilla-Inbound#error0

Comment 19

[Marco Bonardo [:mak]]

(In reply to Wes Kocher (:KWierso) from comment #18)
> Looks like browser-chrome tests got broken too?
> https://tbpl.mozilla.org/php/getParsedLog.php?id=24911878&tree=Mozilla-
> Inbound#error0

ah, Mac only failure. Not sure what the test is doing but sounds like it may be testing localstorage of about blank, I can try reproducing locally.

Comment 20

[Marco Bonardo [:mak]]

I tried debugging the social test on Win (where it passes), and looks like the FrameWorker object is "stealing" the localStorage property from the window containing "resource://gre-resources/hiddenWindow.html", that has a valid scope in localStorage:"secruoser-erg.:resource". Not sure if that is the expected behavior, but it's unclear what the FrameWorker is expected to use as source for the functionality it injects in the sandbox, since it doesn't load anything in the hidden frame before stealing its functionality.
So far I'm not sure why on Mac it ends up trying to copy the localStorage from about:blank, but the global behavior of this thing looks a bit fragile.

while investigating I also found this optimization in the DOM Storage Manager that can likely be removed if we stop supporting about pages: http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/DOMStorageManager.cpp#353 we would indeed fail earlier by not founding a valid scope key.

Comment 21

[Marco Bonardo [:mak]]

apparently also indexedDB would fail in this case, it works in the next test in browser_frameworker only cause the worker is defined as https://example.com/browser/toolkit/components/social/test/browser/worker_social.js rather than a data uri.

Comment 22

[Marco Bonardo [:mak]]

If I load hiddenWindow.html in the iframe (and wait for its load) before proceeding, everything works on Mac as well, so it's apparently fixable, though it's very hairy and I can't still tell why it differs.
Gavin, I see you worked on some of the social FrameWorker stuff, any idea?

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #20)
> I tried debugging the social test on Win (where it passes), and looks like
> the FrameWorker object is "stealing" the localStorage property from the
> window containing "resource://gre-resources/hiddenWindow.html", that has a
> valid scope in localStorage:"secruoser-erg.:resource". Not sure if that is
> the expected behavior, but it's unclear what the FrameWorker is expected to
> use as source for the functionality it injects in the sandbox, since it
> doesn't load anything in the hidden frame before stealing its functionality.

It should be loading the FrameWorker's URI in the iframe before stealing the property. That doesn't sound like the expected behavior - I suppose in the tests we use data: URIs that might end up inheriting the principal of the previously-loaded page, but that should be about:blank of the frameworker's iframe rather than hiddenWindow.html.

> So far I'm not sure why on Mac it ends up trying to copy the localStorage
> from about:blank, but the global behavior of this thing looks a bit fragile.

I guess there might be a race condition - the iframe is inserted into the document (via the call to makeHiddenFrame() in the FrameWorker constructor), then we add a document-element-inserted listener (via the call to load()), and then we load the FrameWorker URL (by setting the "src" attribute on the frame). If the document-element-inserted listener is fired for the initial about:blank load, then we're probably doing the wrong thing here.

Comment 25

[Marco Bonardo [:mak]]

retesting on Try, now that all dependencies should have been fixed
https://tbpl.mozilla.org/?tree=Try&rev=b3af6deac557

Comment 26

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d66af1f8a9

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Marco Castelluccio [:marco]]

"JavaScript error: about:home, line 101: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindow.localStorage]"

With a recent nightly

Comment 29

[Marco Bonardo [:mak]]

as I replied in the other bug, it's the snippets, there's no localStorage usage in about:home