789348 - make about:home not use localStorage

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP

LocalStorage is evil because it does synchronous IO. Also, it called me bad names once when we were in highschool together. In short, it's a doodoo head and eats boogers. So we should punish it by not using it in about:home.

The attached patch is totally untested and not complete. The following needs to be done:

* Remove remaining usage of localStorage from aboutHome.js
* Test that the added functions on IDBFactory actually works

Basically, if we do the first bullet and about:home still works and passes tests, then that verifies the second bullet. But please don't assume that the new function do work.

Jan has graciously offered to help here! Jan, don't hesitate to ask if something looks weird. And let me know if you won't have time for this.

Comment 1

[Marco Bonardo [:mak]]

we are working on this on other bugs, though we didn't have an indexedDB version yet. Actually I don't think we even need a db, we just need a place to store data, cookies would be fine if they'd work on about pages, for example.

Btw please hang a moment since bug 749477 is going to remove at least half of the DOMStorage usage and will bitrot this badly.

Comment 2

[Marco Bonardo [:mak]]

PS: does indexed db work in about pages? since DOMStorage needed special fixes for that I don't expect it to work out of the box.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IndexedDB should work fine out of the box I *think*.

Marco: what's your ETA for this? This is blocking some critical work we are doing for B2G (bug 773373) which we need quite urgently.

We're blocked on this bug because the changes we have for localStorage is making some of the about:home tests fail that test that clearing cookies does not affect localStorage.

Comment 4

[Jan Varga [:janv]]

Man, this is going to be interesting ...

I just figured out that the snippet stuff from the server, for example:
https://snippets.mozilla.com/3/Firefox/18.0a1/20120914060520/Darwin_x86_64-gcc3/en-US/default/Darwin%2011.4.0/default/default/

returns a script that touches local storage (when there are no snippets)
and calls showSnippets()

here's the code:
...
} else {
  localStorage['snippets'] = '';
  showSnippets();
}
...

since I converted the client side to use IDB, this doesn't clear the value anymore
and showSnippets() is called forever (endless loop)

so it's not possible to just do the conversion (LS -> IDB) in the client

the server side should probably call something like clearSnippets()

how can I fix that ?

Comment 5

[Jan Varga [:janv]]

also there's a problem with third party utils checking about:home
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8392

it fails at http://mxr.mozilla.org/mozilla-central/source/content/base/src/ThirdPartyUtil.cpp#277

about:home has no host, so the check fails
the question is, if I can add about: to the third party utils code as another exception besides file://

Comment 6

[Marco Bonardo [:mak]]

(In reply to Jan Varga [:janv] from comment #4)
> Man, this is going to be interesting ...
> 
> I just figured out that the snippet stuff from the server, for example:
> https://snippets.mozilla.com/3/Firefox/18.0a1/20120914060520/Darwin_x86_64-
> gcc3/en-US/default/Darwin%2011.4.0/default/default/
> 
> returns a script that touches local storage (when there are no snippets)
> and calls showSnippets()

Interesting, the original idea was that remote snippets were always including default ones, so if there are not remote snippets they may just return default ones. Probably reusing the page code was seen as an easier solution than to keep remote defaults in sync.
I don't dislike your solution of a clearSnippets(), or better I think it should be called showDefaultSnippets() since that is the scope.

The snippets system has versioning so we can adapt the server side to client changes, just do whatever change you need, bump up the version and notify the maintainer that the remote snippets have to change.  I'm not sure who is managing the remote stuff atm, but that is the easy part to figure out.

Not sure about the third party utils problem, as I said commonly about pages require special code paths :(

Comment 7

[Marco Bonardo [:mak]]

note bug 749477 has likely bitrotted all of this, but should have made it easier.

Comment 8

[Jan Varga [:janv]]

(In reply to Marco Bonardo [:mak] from comment #7)
> note bug 749477 has likely bitrotted all of this, but should have made it
> easier.

no problem, I was using your patch before it landed on m-c

Comment 9

[Jan Varga [:janv]]

Attachment: patch for feedback

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 661207 [details] [diff] [review]
patch for feedback

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

Looks good, but someone that knows this code should review it.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 661207 [details] [diff] [review]
patch for feedback

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

I'd likea bit of refactoring, though let me know if you think I'm going too far away ;)

::: browser/base/content/abouthome/aboutHome.js
@@ +131,5 @@
> +    gDatabase.transaction("store", "readwrite")
> +             .objectStore("store")
> +             .delete("snippets");
> +  }
> +};

What is this supposed to do? it seems to just deletes snippets on each set...

@@ +202,5 @@
> +
> +  let request = indexedDB.open("abouthome", 1);
> +  request.onupgradeneeded = function(event) {
> +    let db = event.target.result;
> +    db.createObjectStore("store");

the version should be moved to a const at the top level, here we should first try to delete an old version of the ObjectStore, then create the new one, so it will support eventual bumps. But see below.

@@ +207,5 @@
> +  };
> +  request.onsuccess = function(event) {
> +    gDatabase = event.target.result;
> +    checkSnippetsLastUpdate();
> +  }

I'd like to have the database initialization handled in its own object/function instead of being mixed in the code like this. Especially since the upgrade path may change over time.
Could be nice to have a separate object that abstracts the underlying storage, you just ask for snippets information and it returns a Map to your callback. Something like:
SnippetsStorage.onStorageReady(function (aMap) { aMap.get("snippets") });
SnippetsStorage.set(aName, aData);
it may cache values internally to avoid hitting the db on multiple calls.
The patch changes then should be more contained (you would invoke loadSnippets only onStorageReady and pass the map to it, as well as showSnippets would be invoked only later, when we already populated the Map).

@@ +228,5 @@
> +    let now = Date.now();
> +    if (lastUpdate && now - lastUpdate <= SNIPPETS_UPDATE_INTERVAL_MS) {
> +      showSnippets();
> +      return;
> +    }

The conditions may be merged as they were? (inverted clearly) is there a reason to separate them?

::: content/base/src/ThirdPartyUtil.cpp
@@ +278,5 @@
>      bool isFileURI = false;
>      aHostURI->SchemeIs("file", &isFileURI);
> +    bool isAboutURI = false;
> +    aHostURI->SchemeIs("about", &isAboutURI);
> +    NS_ENSURE_TRUE(isFileURI || isAboutURI, NS_ERROR_INVALID_ARG);

Unfortunately I can't review this change, you need someone who has knowledge of possible implications of it, especially security side (before you could say "if it succeeds and domain is empty, is a file", but now it may also be an add-on about: page)

Comment 12

[Marco Bonardo [:mak]]

So, I think we should split this bug into 3 parts:
1. Make indexedDB work in about pages (basically the change in ThirdPartyUtil.cpp). Someone who has good knowledge of security implications and content may review that.
2. Abstract about:home storage and make it async-ready. This is what I suggested in my feedback. I may even do that, shouldn't be a problem.
3. replace the storage backend to use indexedDB instead of localStorage (this bug).

I think makes sense to handle these in separate bugs.

Comment 13

[Jan Varga [:janv]]

feel free to take this bug

Comment 14

[Marco Bonardo [:mak]]

(In reply to Jan Varga [:janv] from comment #13)
> feel free to take this bug

OK, will try to reduce the scope here so the patch should be much smaller and we can likely reuse most of your code.

Comment 15

[Honza Bambas (:mayhemer)]

BTW, there soon will be a new localStorage implementation (bug 600307) that may bring a big performance gain.

Comment 16

[Marco Bonardo [:mak]]

Thanks Honza, I'm well aware of that (I also was on the meeting call about it). I'm not sure if the startup costs are also greatly reduced, here we don't have a big usage cost (we just store a couple values) but we want to avoid synchronous startup, so the idea was, since we don't need the data immediately, to use an async storage.

Comment 17

[Honza Bambas (:mayhemer)]

If you don't need the data immediately then there is a great chance that preload of data to the in-memory cache will finish sooner then you use localStorage for the first time.  Then there is no blocking at all.

Also, reads of an origin data from the database always have been relatively fast, according the measured telemetry.

This is for info when making priority decisions on this bug mostly.

Comment 18

[Marco Bonardo [:mak]]

Cool, I guess I may still do the abstraction in bug 820834 to be ready for eventual future changes, or even just adding a small timeout to delay localStorage, if needed.  While the actual switch to indexedDB (this bug) may wait until we get perf data on the new LS that is behind the corner.

Comment 19

[Jan Varga [:janv]]

Well, I think Jonas mentioned to me that the long term plan is to make LS support only temporary storage. Not sure if we want to make an exception just for about:home, probably not.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I think we should try to move chrome code off of localStorage no matter what.

The localStorage changes that Honza is working on are great and will make a big difference. But they are "only" reducing the amount of synchronous IO that we are doing. They can't totally eliminate it.

And any synchronous IO that we are doing from Firefox UI is a bad thing.

So I definitely think that the upcoming changes reduces the urgency in this bug. But it's unknown by how much (we don't have telemetry data yet), and it definitely doesn't *remove* the need for this bug.

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #20)
> The localStorage changes that Honza is working on are great and will make a
> big difference. But they are "only" reducing the amount of synchronous IO
> that we are doing. They can't totally eliminate it.
> 
> And any synchronous IO that we are doing from Firefox UI is a bad thing.

There will only be synchronous wait for data to load into the cache before we first access the storage, it may happen that preload may not be done before that time.  But then there is no I/O on the main thread *at all*.

> 
> So I definitely think that the upcoming changes reduces the urgency in this
> bug. But it's unknown by how much (we don't have telemetry data yet), and it
> definitely doesn't *remove* the need for this bug.

I agree.  We cannot control web content, but we can control our chrome code.

Comment 22

[Marco Bonardo [:mak]]

I'm proceeding in my abstraction of the about:home storage, once that is done we can easily change the underlying storage at any time.

Comment 23

[Marco Bonardo [:mak]]

I will try to update Jan's patch to current code and use indexedDB here for now.

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This uses indexedDB (please ignore dumps), though there are some issues:
1. I'm not sure whether the ThirdPartyUtils.cpp change proposed by Jan is safe
2. Running browser_aboutHome.js test shows 2 issues:
2a. after the first test any indexedDB callback is not invoked anymore, the cursor request is sent but never calls back.
2b. if I don't close the tabs in each test, everything works (?), but there are global dom windows leaking
Experimenting with various timeouts I could still reproduce alternatively a leak, or a not responding indexedDB, or various indexedDB shutdown assertions (should not have pending transactions and such)

I don't currently have the time to dig deeper here, though if you have any idea if I did something very wrong, please let me know, I went through the spec but couldn't find an obvious culprit.

Comment 25

[Marco Bonardo [:mak]]

to reproduce the issue, just apply the patch, recompile content (and libxul clearly) and browser, then run browser_aboutHome.js mochitest-browser test.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 729471 [details] [diff] [review]
patch v1.0

I'll try to look over this before you get back from vacation.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fun. You hit bug 859591.

Comment 28

[Marco Bonardo [:mak]]

(In reply to ben turner [:bent] from comment #27)
> Fun. You hit bug 859591.

cool, double win.

Comment 29

[Marco Bonardo [:mak]]

The fix works, the test doesn't block anymore.
Unfortunately there are still 2 nsGlobalWindow (about:home) leaks notified by the mochitest-browser harness, these happen as soon as a write transaction is executed (and very likely aborted).
I'll see if I can write a smaller mochitest causing the same issue, to investigate.

Comment 30

[Marco Bonardo [:mak]]

adding a setTimeout of even just 50ms at the end of the test makes the leak disappear (A simple setTimeout 0 doesn't). At first glance looks like indexedDB is keeping the window alive until the operation is properly completed (or aborted, that is our case).

Comment 31

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Merged some code suggestions by :bent, moved unrelated code to dependency, added sucky workaround for bug 861308.

Comment 32

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

Comment on attachment 736911 [details] [diff] [review]
patch v1.1

>diff --git a/browser/base/content/test/browser_aboutHome.js b/browser/base/content/test/browser_aboutHome.js

>+    // This is a bad workaround for bug 861308, just give enough time to
>+    // indexedDB to complete transactions :(
>+    yield promiseHackySleep(1);

This seems quite likely to cause intermittent orange on slow/overburdened test machines... I guess we can land it and see pending a solution to that bug :/

Comment 33

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #32)
> This seems quite likely to cause intermittent orange on slow/overburdened
> test machines... I guess we can land it and see pending a solution to that
> bug :/

That's why it's called hacky!
Though I also have an alternative harness suggestion in bug 861308... but you already saw it :)

Comment 34

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Comment 35

[Marco Bonardo [:mak]]

So, I still have one problem, there is a script error reaching the console, with message "AbortError" that the test harness notifies as uncatched error.
It comes from here:
http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IndexedDatabaseManager.cpp#252
And is generated by
let openRequest = indexedDB.open(DATABASE_NAME, DATABASE_VERSION);

The problem is that this
if (NS_FAILED(sgo->HandleScriptError(&event, &status))) {
  NS_WARNING("Failed to dispatch script error event");
  status = nsEventStatus_eIgnore;
}
always fails on abort cause the docshell doesn't exist anymore (I verified that in NS_HandleScriptError the docshell is nullptr), so I can't prevent it from proceeding and printing the error to the console.

I'm not sure if it's correct to log the error if there's no way to first propagate the event, since there's no way to handle the error.
I'd suggest to change the if to use nsEventStatus_eConsumeNoDefault; if the handlers can't be invoked.

Comment 36

[Marco Bonardo [:mak]]

Discussed on IRC, the error is wanted so that developers can see cases where an abort may happen, even though they can't do anything, apart from blocking unload and notify the user "we are saving your data".

Comment 37

[Marco Bonardo [:mak]]

I may try to use expectUncaughtException, though the abort exception is intermittent and async, that makes its usage not funny.
For sure I can remove usage of about:home from sessionstore tests (browser_707862.js and browser_705597.js), it's not needed and it also slows down those without a reason.
The remaining failure is in browser_homeDrop.js

Comment 38

[Zachary Jones]

XSS on about:home using localStorage Injection.

This is not remotely exploitable but I figured I'd submit it anyway as
it could be used to compromise Firefox on a public or shared computer.
FF is storing the HTML for snippets in localStorage. An attacker
could open FF to about:home and quickly make/execute a bookmarklet:

javascript:window.localStorage.setItem('snippets','<iframe
src="https://www.whitehatsec.com" onload="prompt()"
style="width:100%;height:100%;z-index:9999999;position:absolute;left:0px;top:0px;"/>');

This could be expanded further to use a sandboxed iframe to cause the
victim to browse the web inside the iframe while keeping them on
about:home page that has been compromised.

The attacker can then close the browser completely and the about:home
page will remain compromised each time a user opens firefox 

Zach Jones
WhiteHat Security

Comment 39

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

(In reply to Zachary Jones from comment #38)

> Zach Jones
> WhiteHat Security
I've hidden comment 38 for now until the security team can review the issue filled as bug 873769

Comment 40

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

To be clear, the goal of this bug is to move away from localStorage in favor of indexedDB, not to address comment 38/bug 873769.

Comment 41

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

Does this still depend on bug 861308, now that bug 863447 landed?

Comment 42

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #41)
> Does this still depend on bug 861308, now that bug 863447 landed?

nope, I just left the dependency for tracking purpose cause it's still the underlying reason even if we have a workaround.

Comment 43

[Marco Bonardo [:mak]]

Comment on attachment 739625 [details] [diff] [review]
patch v1.2

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

the patch is basically done, I have to fix the testing problem but that will be in a patch apart.

Comment 44

[Marco Bonardo [:mak]]

https://hg.mozilla.org/try/rev/e043ddf353b5

Comment 45

[Marco Bonardo [:mak]]

ehr https://tbpl.mozilla.org/?tree=Try&rev=e043ddf353b5

Comment 46

[Marco Bonardo [:mak]]

Attachment: patch v1.3

Minor unbitrot.
This can land at any time in inbound, I already landed the tests changes that allow it to not fail tests.
I'm traveling today though, so won't be able to land it in case.

Comment 47

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73e06fde62e3

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73e06fde62e3

Comment 49

[Jeferson Hultmann]

> let openRequest = indexedDB.open(DATABASE_NAME, DATABASE_VERSION);

indexedDB is not always available due to bug 781982.

Comment 50

[Marco Bonardo [:mak]]

(In reply to Jeferson Hultmann from comment #49)
> > let openRequest = indexedDB.open(DATABASE_NAME, DATABASE_VERSION);
> 
> indexedDB is not always available due to bug 781982.

that's fine, the page doesn't really care.

Comment 51

[Marco Castelluccio [:marco]]

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

Worth a new bug?

Comment 52

[Marco Bonardo [:mak]]

Could be invoked by the snippets code, the page surely doesn't invoke localStorage anywhere.
Please file it and needinfo mkelly

Comment 53

[Matthew N. [:MattN]]

(In reply to Marco Bonardo [:mak] (Away Aug 07-11) from comment #52)
> Please file it and needinfo mkelly

Filed as bug 900918 for those that are looking for it.