Closed Bug 529922 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox :: Private Browsing, defect)

3.6 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a5

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 3 obsolete files)

Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attachment #413428 - Flags: review?(vladimir)
Summary: Improve the test for bug 529667 on 1.9.2 → Fix the test for bug 529667 on 1.9.2
Attachment #413428 - Flags: review?(vladimir) → review?(gavin.sharp)
Attachment #413428 - Flags: review?(gavin.sharp) → review+
Landed this on 1.9.2:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/61ca95389aa2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [does not need trunk landing]
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.
Flags: in-testsuite+
Resolution: FIXED → WONTFIX
Whiteboard: [does not need trunk landing]
Oh, it went orange because the setTimeout(delayedStartup) happens before you replace it, presumably. I should have thought of that!
(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.
(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?
Wouldn't changing the nested executeSoon calls to a setTimeout(..., 100) be an easier way to solve this?
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.
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... :/
delayedStartup does a couple of things that could be checked, but it all seems fragile... Maybe check whether win.document.activeElement == win.gURLBar.inputField?
Attached patch Make sure delayedStartup is run (obsolete) — Splinter Review
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.
Attachment #449151 - Flags: review?(dao)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attached patch Patch (v2) (obsolete) — Splinter Review
I've also submitted this patch to the try server to make sure that it doesn't break anything.
Attachment #413428 - Attachment is obsolete: true
Attachment #449156 - Flags: review?(dao)
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.
Attachment #449156 - Flags: review?(dao)
Attachment #449151 - Flags: review?(dao) → review?(gavin.sharp)
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.
Attachment #449156 - Flags: review?(gavin.sharp)
(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.
Status: REOPENED → ASSIGNED
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.
Attachment #449151 - Flags: review?(gavin.sharp) → review+
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)
(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.  :-)
Attached patch trunk patchSplinter Review
Attachment #449151 - Attachment is obsolete: true
Attachment #450179 - Flags: review?(gavin.sharp)
Attachment #450179 - Flags: review?(gavin.sharp) → review+
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"?
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?
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: Fix the test for bug 529667 on 1.9.2 → Improve the test for bug 529667 to make sure it's running after delayedStartup
Target Milestone: --- → Firefox 3.7a5
Gavin: ping?
Sure, same approach on branch seems fine.
Attachment #449156 - Attachment is obsolete: true
Attachment #449156 - Flags: review?(gavin.sharp)
Duplicate of this bug: 364304
Need to document browser-delayed-startup-finished
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.