529922 - Improve the test for bug 529667 to make sure it's running after delayedStartup

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

Spawn from bug 529667 comment 8.

This replaces delayedStartup instead of relying on a couple of executeSoon calls.

Note that this is only needed on 1.9.2, since bug 512645 on trunk makes sure that the correct behavior can be expected there.

Comment 1

[(no longer active)]

Landed this on 1.9.2:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/61ca95389aa2

Comment 2

[(no longer active)]

This turned our browser-chrome PB tests orange.  The patch must have bitrotted since I wrote it for some reason.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1275424537.1275425496.24006.gz

I backed it out:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2cb83e106cf7

I don't think this is worth me spending any more time on.  We haven't had intermittent oranges because of this on 1.9.2.  So I'm WONTFIXing this until we have an evidence that this deserves someone working on, and I'll take it again at that point.

Comment 3

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

Oh, it went orange because the setTimeout(delayedStartup) happens before you replace it, presumably. I should have thought of that!

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #2)
> The patch must have bitrotted since I wrote it for some reason.

There has been no change to that file since its initial landing.

The test gives a false sense of validity, so it should definitely be fixed or removed.

Comment 5

[(no longer active)]

(In reply to comment #3)
> Oh, it went orange because the setTimeout(delayedStartup) happens before you
> replace it, presumably. I should have thought of that!

Hmm, right.  But I assume that accessing delayedStartup before the load event is not safe, right?  So what can be done here?

Comment 6

[(no longer active)]

Wouldn't changing the nested executeSoon calls to a setTimeout(..., 100) be an easier way to solve this?

Comment 7

[Dão Gottwald [:dao]]

Probably... but it's impossible to tell this from just running the test, since it passes if it fails to wait for delayedStartup. If you can make it report a failure in that case, that would be a good fix for trunk as well.

Comment 8

[(no longer active)]

How would I do that?  The only way that I can think of is adding something like |window.didMyDelayedStartup = true;| at the end of delayedStartup and check for that in the test, but that sounds really dirty... :/

Comment 9

[Dão Gottwald [:dao]]

delayedStartup does a couple of things that could be checked, but it all seems fragile... Maybe check whether win.document.activeElement == win.gURLBar.inputField?

Comment 10

[(no longer active)]

Attachment: Make sure delayedStartup is run

The focused element would be the browser element, not the URL bar, and that happens inside the BrowserStartup function.

I decided to see if gPrivateBrowsingUI is initialized.  This object is initialized at delayedStartup, and it's at least relevant to private browsing.

I'll prepare a branch patch as well.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

I've also submitted this patch to the try server to make sure that it doesn't break anything.

Comment 12

[(no longer active)]

Comment on attachment 449156 [details] [diff] [review]
Patch (v2)

Try server results were not promising yet.  I'm doing a local build to try to reproduce and fix the problem.

Comment 13

[Dão Gottwald [:dao]]

Does DOMContentLoaded work? It should put you before BrowserStartup.
The problem with _privateBrowsingService is that it could be a smart getter (i.e. it's not forward compatible).

I'm not actually sure what went wrong with the first patch, given that browser_bug495058.js works fine.

Comment 14

[(no longer active)]

(In reply to comment #13)
> Does DOMContentLoaded work? It should put you before BrowserStartup.
> The problem with _privateBrowsingService is that it could be a smart getter
> (i.e. it's not forward compatible).

This is a problem that I don't want to solve here.  This should work correctly on trunk even without that sanity patch (I'm in fact fine with just not taking the trunk patch at all), and we're not going to change _privateBrowsingService to a smart getter on branch.

> I'm not actually sure what went wrong with the first patch, given that
> browser_bug495058.js works fine.

I'm not sure, but the second patch WFM locally, although it didn't on the try server.

Comment 15

[Dão Gottwald [:dao]]

I'd rather see this hardened on trunk as well. The nested executeSoons seem fragile. E.g. I think there were considerations of delaying delayedStartup further.

Comment 16

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

Comment on attachment 449151 [details] [diff] [review]
Make sure delayedStartup is run

(though I agree that this is fragile - I wouldn't mind adding a notifyObservers() to the end of delayedStartup)

Comment 17

[(no longer active)]

(In reply to comment #16)
> (From update of attachment 449151 [details] [diff] [review])
> (though I agree that this is fragile - I wouldn't mind adding a
> notifyObservers() to the end of delayedStartup)

OK, maybe I'll just bite the bullet and do that.  :-)

Comment 18

[(no longer active)]

Attachment: trunk patch

Comment 19

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

Comment on attachment 450179 [details] [diff] [review]
trunk patch

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_newwindow_stopcmd.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_newwindow_stopcmd.js

>   let win = OpenBrowserWindow();

>+  Services.obs.addObserver(function(subject, topic, data) {
>+    Services.obs.removeObserver(arguments.callee, "browser-delayed-startup-finished");
>+    var win = subject.QueryInterface(Ci.nsIDOMWindow);

name this notifiedWin, and sanity check it against the closed-over "win"?

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4e6e91e0a42b
http://hg.mozilla.org/mozilla-central/rev/41b7a6acaa16

Gavin, do you want the same approach on branch as well?

Comment 21

[(no longer active)]

Gavin: ping?

Comment 22

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

Sure, same approach on branch seems fine.

Comment 24

[Eric Shepherd [:sheppy]]

Need to document browser-delayed-startup-finished