598264 - about:home search does not work if cookies behavior is "ask me every time" or "reject"

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[David (Yavatar)]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b7pre) Gecko/20100920 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0b7pre) Gecko/20100920 Firefox/4.0b7pre

If you goto default home page ( about:home ), the page looks similar to a Firefox search page but hitting <ENTER> or clicking on 'Search' does nothing.

Either the page needs to be updated to allow Search or use the default Firefox (non-nightly) Search page. 

Alternatively use Minefield start page (http://www.mozilla.org/projects/minefield/) for default home page. This may actually be more appropriate for Minefield.

I thought that either the Minefield start page or the Pre-release page was the default home page before because I do not recall seeing about:home (in general or as the default home page) previous to pre7.

Reproducible: Always

Steps to Reproduce:
1. Goto about:home
2. Type in anything in search box
3. Try to start a search

Comment 1

[John P Baker]

This WFM on a new profile; But if I set 'Ask me every time' option for Cookies I get:

Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "chrome://browser/content/aboutHome.js Line: 100"]

100   gSearchEngine = JSON.parse(localStorage["search-engine"]);

and thus don't get any information about the search engine.

Comment 2

[Marco Bonardo [:mak]]

David, do you have that cookies setting enabled? did you do anything special before noticing the problem?

Comment 3

[Marco Bonardo [:mak]]

so I can confirm there are 2 things that can block us here, one is network.cookie.cookieBehavior set to "reject" or "ask me every time" the other one is setting dom.storage.enabled to false.

Comment 4

[Marco Bonardo [:mak]]

I fear this should block.

Comment 5

[David (Yavatar)]

It's at least something with the cookies(I clear Google from History each time to test):

Accept / ... / Until Expire - OK

Accept / ... / Session Expire - OK

Accept / ... / Ask Me Every - NO


Checked dom.storage.enabled too which is OK (TRUE)


I almost missed it, but the Google logo and search/advanced options don't pop up unless cookies are in place as well as the "Thanks for choosing Firefox! To get the most out of your browser, learn more about the latest features.". If you want screen shots, I can do it later.

Out of pure curiosity, after doing a search, I turned off cookies and it went back to the same behavior, so no auto cookies, no search. :/

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This moves the about check to a static method, and uses it to check in both places (when getting localStorage, and when invoking CanUseStorage).

I've added a test that ensures I have not regressed old cookie behavior for other pages (looks like this test was missing), and a test for the chrome caller case. Unfortunately I have no idea how to test about: pages without using about:home that is a browser feature or adding a specialized page. So I'll add a test in browser/content for it in the next patch below.

I think we don't care much about dom.storage.enabled case. It's a really edge-case and I don't think many users could need to play with that pref, plus if they do I imagine there are serious reasons to do it, reasons that we don't want to circumvent even if lose functionality.

Comment 7

[Marco Bonardo [:mak]]

Attachment: browser test v1.0

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 477292 [details] [diff] [review]
browser test v1.0

assigning review to dolske for now, but feel free to renominate if needed.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 477292 [details] [diff] [review]
browser test v1.0

>+  desc: "Check that rejecting cookies does not prevent page from working",
>+  setup: function ()
>+  {
>+    Services.prefs.setIntPref("network.cookies.cookieBehavior", 2);
>+  },

I think you'll want a registerCleanupFunction() here, to ensure the various prefs get reset after the test.


>+    ok(Array.some(snippetsElt.getElementsByTagName("span"), function(elt) {
>+      return elt.hidden;
>+    }), "A default snippet is visible.");

Shouldn't that be |!elt.hidden|?


>+  // browser-chrome test harness inits browser specifying an hardcoded page
>+  // and this causes defaultArgs to not be evaluated.  To populate about:home
>+  // we need it to be invoked, so do it right now.
>+  Cc["@mozilla.org/browser/clh;1"].getService(Ci.nsIBrowserHandler).defaultArgs;

Why's that needed?

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> >+    ok(Array.some(snippetsElt.getElementsByTagName("span"), function(elt) {
> >+      return elt.hidden;
> >+    }), "A default snippet is visible.");
> 
> Shouldn't that be |!elt.hidden|?

ugh, stupid negations.

> >+  // browser-chrome test harness inits browser specifying an hardcoded page
> >+  // and this causes defaultArgs to not be evaluated.  To populate about:home
> >+  // we need it to be invoked, so do it right now.
> >+  Cc["@mozilla.org/browser/clh;1"].getService(Ci.nsIBrowserHandler).defaultArgs;
> 
> Why's that needed?

I'll try to clarify the comment. btw about:home is populated when we evaluate homepage overrides. browser-chrome harness does not evaluate overrides because browser is invoked on the command line with about:blank as argument. When browser is invoked with a fixed page in arguments browserContentHandler does not evaluate defaultArgs for the browser window (That includes homepage overrides)

Comment 11

[Marco Bonardo [:mak]]

Attachment: browser test v1.1

Comment 12

[David (Yavatar)]

Instead of going to a web site, why are we referencing in internally now? It looks like about:home search bug is at least in Beta 5 if not further back. The only advantage I see is if you're offline, but then you wouldn't be doing a search from it anyways.

Incidentally, why isn't there an about:firefox (or Firefox) that goes to the same place or just bring up the about Window? Seems like it make sense than about:home ... You'd think about:home be like: Home: this is where you go to sleep after being out all day, unless you are a Mozilla developer and home is wherever you can put your head down. Just a thought.

Comment 13

[Marco Bonardo [:mak]]

(In reply to comment #12)
> Instead of going to a web site, why are we referencing in internally now?

Because we want to stop referencing an externally hosted page that limits some of the stuff we want to do. This is not the right place to discuss the feature though (mozilla.dev.apps.firefox would be better, but there is really not much to discuss, we need that page to be internal, that's all).
Regarding bugs, they can be fixed.

Comment 14

[David (Yavatar)]

O-I-C ... =)

Wouldn't have caught the bug if the homepage default change hadn't been made. Unfortunately I already took off the older Beta versions so I can't check back to Beta 1. I was doing some benchmarking to see how they had come along but I finished that already. I still had a beta 5 at work I still had to benchmark so I saw it's there too. Not sure if this an issue with 3.6 too? Do you need to know for 3.6 too?

Comment 15

[Marco Bonardo [:mak]]

3.6 will keep using the old google hosted page, for now.

Comment 16

[David (Yavatar)]

Invalid URL. Hrm, I guess it is just with the beta/nightly. =)

(You beat me to the post when I went to get some cereal for the kids. ;))

Comment 17

[Honza Bambas (:mayhemer)]

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

>diff --git a/dom/src/storage/nsDOMStorage.cpp b/dom/src/storage/nsDOMStorage.cpp
>@@ -789,18 +786,20 @@ nsDOMStorage::CanUseStorage(PRPackedBool
>+    if (!URICanUseChromePersist(subjectURI) &&
>+        (cookieBehavior == BEHAVIOR_REJECT || lifetimePolicy == ASK_BEFORE_ACCEPT))

Put !URICanUseChromePersist() after the cookie state conditions, you are checking URICanUseChromePersist even it's value might not be needed when cookie setting is positive for us.  CanUseStorage is called very often, this may have some performance impact, though small.

>diff --git a/dom/tests/mochitest/localstorage/Makefile.in b/dom/tests/mochitest/localstorage/Makefile.in
>     test_removeOwnersAPISessionOnly.html \
>     test_storageConstructor.html \
>+    test_localStorageCookieSettings.html \
>     $(NULL)

Keep the file list alphabetically sorted please.

>diff --git a/dom/tests/mochitest/localstorage/test_localStorageCookieSettings.html b/dom/tests/mochitest/localstorage
>+try {
>+  localStorage.setItem("contentkey", "test-value");
>+  value = localStorage.getItem("contentkey");
>+} catch(ex if (ex.name == "NS_ERROR_DOM_SECURITY_ERR")) {
>+  ok(true, "localStorage unable to set item");
>+}

When your patch would have a flaw and would allow setting an item, your test won't detect it.  You should check that the result is exactly and only NS_ERROR_DOM_SECURITY_ERR, if anything else happens, as well as no exception at all, you have to fail the test, right?

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> Put !URICanUseChromePersist() after the cookie state conditions

yep, makes sense for perf since the common case is active cookies.

> >diff --git a/dom/tests/mochitest/localstorage/Makefile.in b/dom/tests/mochitest/localstorage/Makefile.in

> Keep the file list alphabetically sorted please.


sorry I did not notice it was sorted :\

> >diff --git a/dom/tests/mochitest/localstorage/test_localStorageCookieSettings.html b/dom/tests/mochitest/localstorage
> >+try {
> >+  localStorage.setItem("contentkey", "test-value");
> >+  value = localStorage.getItem("contentkey");
> >+} catch(ex if (ex.name == "NS_ERROR_DOM_SECURITY_ERR")) {
> >+  ok(true, "localStorage unable to set item");
> >+}
> 
> When your patch would have a flaw and would allow setting an item, your test
> won't detect it.  You should check that the result is exactly and only
> NS_ERROR_DOM_SECURITY_ERR, if anything else happens, as well as no exception at
> all, you have to fail the test, right?

Hum, I expect an unhandled exception to be considered a test failure (it not I'll cry)... but yeah if there is no exception we should fail, I'll check value in a finally.

Comment 19

[Marco Bonardo [:mak]]

Attachment: patch v1.1

addressed comments

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v1.2

the test was timing out due to a wrong src path, fixed.

Comment 21

[Honza Bambas (:mayhemer)]

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

I still don't think that test_localStorageCookieSettings.html is at it should be.

- you should do this:
try {
  localStorage.setItem("a", "b");
  ok(false);
}
catch (ex) {
  is(ex.name, "DOM_SEC_ERROR");
}

- check for getItem is null then is imho not really needed, but not bad to have it

- you have to clean up preferences you have changed when an exception is threw during your test, otherwise it effects following tests


Otherwise ok.

r=honzab with those changes.

Comment 22

[Marco Bonardo [:mak]]

Attachment: patch v1.3

Addressed comments.
I've removed the null check since getItem is throwing the dom sec error as well, so keeping them was adding a bunch of useless code that would have made the test scope less clear.
Since everything that could throw is inside a try catch, that now catches all exceptions, the clearUserPref calls should be fine to clean up preferences.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.4

Sorry I attached the wrong patch ;( this is the right one.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 480673 [details] [diff] [review]
patch v1.4

r+ for the test.

Comment 25

[Marco Bonardo [:mak]]

about:home b-c test
http://hg.mozilla.org/mozilla-central/rev/8f3b9c2081cf
patch v1.4
http://hg.mozilla.org/mozilla-central/rev/e5aa3e35e323