749477 - Searching from about:home broken if chromeappsstore.sqlite is deleted from profile folder.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

Someone complained on a forum that their about:home search box had been hijacked by Yahoo, wanting it to go back to the default Google search.
Knowing that the chromeappsstore.sqlite file is where that setting is stored and that the file would be regenerated on the next startup, I recommended that the person move the file out of the profile folder to their desktop while Firefox was not running, then restart Firefox to regenerate the file.

The file WAS regenerated, but now the search box stopped working entirely.

I tried to reproduce this with my profile, and saw this error in the error console when I loaded about:home after deleting the file and restarting Firefox:

Timestamp: 4/26/2012 8:02:47 PM
Error: SyntaxError: JSON.parse: unexpected character
Source File: chrome://browser/content/abouthome/aboutHome.js
Line: 181


The affected line of code is: 
  gSearchEngine = JSON.parse(localStorage["search-engine"]);


This is probably related to bug 637561, bug 645189, bug 637639, and/or bug 615785 (would this error go away if I waited for Firefox to upgrade to a newer version?).

Expected results: Reset DOM storage when chromeappsstore.sqlite gets regenerated (or just revert back to a default engine when that JSON.parse() fails).


I can upload both the original chromeappsstore.sqlite and the regenerated one if that'd help.

Comment 1

[Marco Bonardo [:mak]]

Removing random files from the profile is hardly a solution to problems.
Actually yahoo didn't hijack anything, the user installed something that asked him if he wanted to make yahoo the default search engine, and he accepted. We use the default search engine in about:home.
Btw, reset the default search engine to whatever he wants, reset browser.startup.homepage_override.buildID (to simulate installing a new version) and restart the browser.

Comment 2

[Justin Dolske [:Dolske]]

We should still recover from this... Does some init code just need better error handling?

Comment 3

[Marco Bonardo [:mak]]

I think we may move all the about:home feeding code from BrowserContentHandler to the load handler we already have in browser.js
This way we'd pay a brief cost when the page is loaded, but would solve all of those cases where localStorage is an issue. The snippets would remain in localStorage but we could slightly delay their refresh just to improve startup

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Well, this doesn't remove completely localStorage usage, though it is delayed, so likely will come after first paint.  We may further delay loadSnippets too if it's still found problematic.
This should fix other bugs too, related to the search engine not working in about:home, and default engine changes not reflected till a new version.  The downside is that the engine logo may appear a bit later, but we were already using load before and nobody noticed (we moved to DOMContentLoad mostly for coherence).
I explicitly avoided adding a load listener in browser.js, cause would be a useless perf hit, so I had to hack around the ones we have, that unfortunately fires after "load".
While fixing this I also found we are handling load and clicks twice in about pages :(

Comment 5

[Marco Bonardo [:mak]]

self-comment: forgot a "let" before gObserver :/

Comment 6

[Frank Yan (:fryn)]

I actually don't understand why we're using localStorage (which I have heard to be performance-unfriendly) and such complicated code to load a search engine into about:home. Could you explain?

If it has to do with about:home being unprivileged, when we fold about:home's content into about:newtab, the combined page will be privileged (with snippets in an unprivileged <iframe/> or something), so I hope we get rid of any unnecessary complexity.

(In reply to Marco Bonardo [:mak] from comment #4)
> While fixing this I also found we are handling load and clicks twice in
> about pages :(

Yes, I discovered that a few days ago too.
Any idea why that is happening or how to fix it?

Comment 7

[Marco Bonardo [:mak]]

(In reply to Frank Yan (:fryn) from comment #6)
> I actually don't understand why we're using localStorage (which I have heard
> to be performance-unfriendly) and such complicated code to load a search
> engine into about:home. Could you explain?

That's what I'm fixing here, so I don't get the question.
Regardless, we still need a place where to store snippets contents, cookies can't be used in about pages, and passing snippets scripts from chrome to content may be subject to some additional security requirements, thus they should be fetched and stored in content privileges if possible, that is what I didn't touch yet.  About:home was made unpriviledged cause the chrome requirements were really low (just a couple strings need to be passed) and it was the fastest path to have the feature in the product (type="content" iframes were still buggy at that time and not exactly safe, Mossop fixed them later for about:addons).

> If it has to do with about:home being unprivileged, when we fold
> about:home's content into about:newtab...

No doubts, though that's future, while this is what I think we should do now.

> (In reply to Marco Bonardo [:mak] from comment #4)
> > While fixing this I also found we are handling load and clicks twice in
> > about pages :(
> 
> Yes, I discovered that a few days ago too.
> Any idea why that is happening or how to fix it?

Just read the nsIWebProgressListener documentation, it's expected that we get 2 STATE_STOP, we just didn't handle it.

Comment 8

[Marco Bonardo [:mak]]

btw, the patch has a fix for the double handling.

Comment 9

[Frank Yan (:fryn)]

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

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

::: browser/base/content/browser.js
@@ +5221,3 @@
>  
>      if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
> +        aStateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW &&

When I discovered this bug, I looked through all the state flags and hoped this would work too. Unfortunately, this works for about:home, but it breaks about:certerror when displayed for pages like https://johnath.com

Comment 10

[Marco Bonardo [:mak]]

(In reply to Frank Yan (:fryn) from comment #9)
> When I discovered this bug, I looked through all the state flags and hoped
> this would work too. Unfortunately, this works for about:home, but it breaks
> about:certerror when displayed for pages like https://johnath.com

Uh I wonder why that differs, will have to debug a bit.

Comment 11

[Frank Yan (:fryn)]

(In reply to Marco Bonardo [:mak] from comment #7)

Thank you for the thorough and thoughtful explanations, by the way.
I simply wasn't aware of the history of the page, so I wanted to understand why it was built that way, and now I do.

Comment 12

[Marco Bonardo [:mak]]

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

I've unbitrotted the patch and investigating on the network issue, as it is this version is not useful.

Comment 13

[Marco Bonardo [:mak]]

just some logging to share, on STATE_STOP

about:neterror
IS_NETWORK 0, IS_WINDOW 0, IS_DOCUMENT 0
IS_NETWORK 0, IS_WINDOW 0, IS_DOCUMENT 0

about:certerror
IS_NETWORK 1, IS_WINDOW 1, IS_DOCUMENT 0

about:newtab
IS_NETWORK 1, IS_WNDOW 0, IS_DOCUMENT 1

about:home
IS_NETWORK 1, IS_WNDOW 1, IS_DOCUMENT 0
IS_NETWORK 0, IS_WNDOW 0, IS_DOCUMENT 0

So, likely this is not a reliable source of discriminating multiple calls.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.1

this is an alternative and simpler way to avoid double handling, I just store an attribute in the documentElement and check for it. Afaict it's working fine.
Not yet asking for review cause I want to see if I can improve the lazy loading to make it able to handle asynchronous search service initialization (bug 785487).
To clarify, I don't want to fix that bug here, just to make the code "ready" for it, as far as possible.

Comment 15

[Marco Bonardo [:mak]]

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

Frank, what do you think of this simple approach?

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v1.2

This one should be good for reviews too.

Comment 17

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

fryn: any ideas when you'll be able to get to this? If you're too busy with Metro stuff we should find someone else to review, this blocks a lot of good work.

Comment 18

[Frank Yan (:fryn)]

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

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> fryn: any ideas when you'll be able to get to this?

Done.

Comment 19

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

Thanks!

Comment 20

[Marco Bonardo [:mak]]

I am fixing the b-c test, cause it's built around storage and will fail badly.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.3

fixed test, not going to reask for review on this since these changes are really uninteresting

Comment 22

[Marco Bonardo [:mak]]

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

Comment 23

[Marco Bonardo [:mak]]

backed out cause browser/base/content/test/browser_bug435325.js started failing intermittently with

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug435325.js | After clicking the Try Again button, we're back online. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/30518d30166e

So looks like for some reason, sometimes, the click listener is not properly added. The difference with this patch is that previously we were trying to add the listener twice, while now just once.
That test hits about:neterror, so that page has something "special", or there is some timing issue related to when we add the listener (too early maybe, before we were re-adding it at the second STOP, now we always add it at the first one, so it may not be "ready" for it yet?).
Will do some testing on try server.

Comment 24

[Marco Bonardo [:mak]]

about:netError is indeed special cause it's a background load, that doesn't even fire the usual load notification (the first sentence here is quite clear https://bugzilla.mozilla.org/show_bug.cgi?id=568434#c5).
Also, we basically skip the STATUS_STOP notification with documentURI == about:netError, cause we check Components.isSuccessCode(aStatus), that is false for it. We are instead installing listeners on the previously generated about:blank :(
I still don't have a proper explanation for the random failure, but I suspect it has something to do with the timing at which the about:blank document is replaced and pagehides are fired. As it was before we were basically continuously adding listeners to each new invokation.

I may remove the isSuccessCode check, filter out about:blank pages, and properly wait for the STATE_STOP of the expected error url, but it comes after DOMContentLoaded and the test can't use the load event cause it's not fired.

Comment 25

[Marco Bonardo [:mak]]

Though, if that fix would be acceptable (so properly wait for the correct page and stop relying on DOMContentLoad) we may just listen for the attribute in the test. At least this would remove some work from about:blank.

Comment 26

[Marco Bonardo [:mak]]

Attachment: proposal 1

so basically, something like this

Comment 27

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

Can we land this without the fix for double-adding the listeners, and figure that out in a followup?

Comment 28

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #27)
> Can we land this without the fix for double-adding the listeners, and figure
> that out in a followup?

I think so, I will do some testing cause I remember I had to fix the issue in the original patch, but the current patch should not suffer the problem, by visual inspection.
Though, with all the planned changes this bug is blocking (especially the async search service change) double handlers may create more hidden bugs than we expect, plus the efficiency problem on about:blank, but this exists already.

Btw, just to be sure this is the try result with the 2 patches, showing no random failures.
https://tbpl.mozilla.org/?tree=Try&rev=3f43a67fcc9a

Comment 29

[[e-mail]]

(In reply to Marco Bonardo from comment #13)
> just some logging to share, on STATE_STOP
> 
> about:neterror
> IS_NETWORK 0, IS_WINDOW 0, IS_DOCUMENT 0
> IS_NETWORK 0, IS_WINDOW 0, IS_DOCUMENT 0

In my SeaMonkey port of the offline error page bug, I originally wanted to use a location change web progress notification, because it conveniently supplies a flag to indicate that an error page is being loaded. Unfortunately it fires before the document is created, so we can't add any event listeners yet.

I toyed with the idea of network web progress notifications, but I couldn't see any way of filtering them down (as you point out, they don't seem to set either the IS_WINDOW or IS_DOCUMENT flags on which they could be filtered) so I plumped for DOMContentLoaded events because there wouldn't be so many of them.

Comment 30

[Marco Bonardo [:mak]]

yes adding a DOMContentLoaded listener may work as well, though since we already have a sort of notification it's a pity to add further work, btw I think we can discuss this in the follow-up and maybe sync the code.

Comment 31

[Marco Bonardo [:mak]]

btw note that if we ignore the about:blank pages (and we should for perf reasons) DOMContentLoaded already fired when we reach the last STATE_STOP.

Comment 32

[Marco Bonardo [:mak]]

Attachment: reduced patch v1

ok, this is the reduced patch, I just removed the changes to browser.js section that is adding listeners, will coalesce them with browser_bug435325.js changes to the follow-up I'm going to file.
I tested this manually and run the tests locally, so I'll just push this, there is no new code added in need for a review.

Comment 33

[Marco Bonardo [:mak]]

the follow-up is bug 790934, any idea/suggestion is welcome.  Neil could you please point out there the Seamonkey code, we may eventually evaluate syncing those.

Comment 34

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08c90c0d3f55

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08c90c0d3f55

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08c90c0d3f55