If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

browser_privatebrowsing_openlocation.js hangs when I run it on Ubuntu

RESOLVED FIXED in Firefox 3.7a5

Status

()

Firefox
Private Browsing
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: ted, Assigned: Ehsan)

Tracking

Trunk
Firefox 3.7a5
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

I've actually been seeing this for a while, but I forgot to file it. When I run browser-chrome tests, browser_privatebrowsing_openlocation.js hangs with the "Open Web Location" dialog just sitting there. It passes one test, then hangs. The output looks like:

Waiting for window activation...
Running chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js...
TEST-PASS | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js | The input field should be correctly auto-filled
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js | Timed out

Looking at the Error Console while the test is hung, I see:
Error: input is null
Source File: chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js
Line: 69
Do you always get this behavior?  Does it happen the first time that the dialog is opened?  (A full test should open it 5 times.)

The input variable comes from here: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js#68>, given the fact that the code is being executed from the load handler, I would expect the DOM tree to be initialized by then...

I don't have a lot of ideas, really...
Another thing to try:

Load chrome://browser/content/openLocation.xul, and type the following into the location bar:

javascript:alert(document.getElementById("dialog.input"));
Yes, this happens 100% of the time for me, AFAICT. I only see one dialog, and then the test times out.

If I load that in a tab and then the javascript URL, I get an [object XULElement], as expected.

I'm on 64-bit Ubuntu, if it makes any difference.
Created attachment 423068 [details] [diff] [review]
Dump the DOM tree

Can you apply this patch and send back the results, so that we can get some insight on what's going on inside the DOM tree?
Sorry, forgot about this, building with this patch now.
Created attachment 424232 [details]
log output

Here's the output with that patch.
OK, this is fun!

The first domwindowopened notification is observed for the error console, and the second one for a view source window!

Do you actually see these windows opened?  What happens if you run only this test?  What happens if you only run private browsing tests?
Oh, I opened those manually. (Oops) I think if I don't open those, I don't actually get any output(?!) I was running this test standalone. Running just the private browsing tests gives the same result.
(In reply to comment #8)
> Oh, I opened those manually. (Oops) I think if I don't open those, I don't
> actually get any output(?!) I was running this test standalone. Running just
> the private browsing tests gives the same result.

Have you actually tried not opening any windows manually?  Doesn't it generate any output?

Something is very very wrong here.  :-/
No, if I don't open the error console, I get no output. I guess that means the test isn't getting a domwindowopened notification? The "Open Web Location" dialog definitely shows up.
This is really difficult to debug remotely.  Can you please experiment with adding some timeouts in different places to see if this is a timing issue.

Oh, and another thing to try is running this test in 1.9.2 and 1.9.1 as well?

Updated

8 years ago
Blocks: 554934
Duplicate of this bug: 558538
We have a log from one of the Fedora unit test boxes.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270892665.1270893238.22014.gz
Blocks: 438871
Whiteboard: [orange]
Created attachment 438294 [details] [diff] [review]
work in progress?

I can reproduce the bug.  This fixes the timeouts, but some of the time (2 out of 3 tries so far) I see other failures in the test instead of the timeouts -- and extra "Open Location" dialogs hanging around after the test has run.
No longer blocks: 438871
Whiteboard: [orange]
(In reply to comment #14)
> Created an attachment (id=438294) [details]
> work in progress?
> 
> I can reproduce the bug.  This fixes the timeouts, but some of the time (2 out
> of 3 tries so far) I see other failures in the test instead of the timeouts --
> and extra "Open Location" dialogs hanging around after the test has run.

Hmm, this seems to be the correct thing to do here.

What are the other failures?  Do you have a log?
Created attachment 438302 [details]
log of failures with previous patch
Created attachment 438403 [details] [diff] [review]
Patch

I _think_ this should solve the problem.  dbaron, would you mind testing this please?
Comment on attachment 438403 [details] [diff] [review]
Patch

I finally managed to reproduce the problem in a Fedora 12 VM, and verified that this patch fixes the problem.

Requesting Gavin for review.
Attachment #438403 - Flags: review?(gavin.sharp)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Blocks: 558538
Blocks: 517791
Comment on attachment 438403 [details] [diff] [review]
Patch

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

>             browser.addEventListener("load", function() {

>+              SimpleTest.waitForFocus(callback);

This waits for focus on the global |window|, right? What ensures that that happens?
(In reply to comment #19)
> (From update of attachment 438403 [details] [diff] [review])
> >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js
> 
> >             browser.addEventListener("load", function() {
> 
> >+              SimpleTest.waitForFocus(callback);
> 
> This waits for focus on the global |window|, right?

Yes.

> What ensures that that happens?

When the open location dialog is closed, the previous window should be focused.  If that doesn't happen, (like in case where another window on the system was put to foreground right before the open location dialog was opened), I guess we want the test to time out, because we generally want to keep the test window focued, right?
(In reply to comment #20)
> When the open location dialog is closed, the previous window should be focused.

Can't that happen before the page load completes? This waitForFocus() call is inside the browser's load handler...
(In reply to comment #21)
> (In reply to comment #20)
> > When the open location dialog is closed, the previous window should be focused.
> 
> Can't that happen before the page load completes? This waitForFocus() call is
> inside the browser's load handler...

Doesn't the load handler get invoked when the browser receives the load event from the page?  If yes, then by that time the page load must have been completed.
My point was that we're potentially calling waitForFocus() after the window is already focused. What I'd forgotten is that it handles that case by just calling the callback immediately... the method name is kind of confusing since it doesn't necessarily "wait" for a focus event.

Why do we care about focus in that case, though? I seems like it turns out to just be a roundabout way of calling executeSoon() like we did before, and so it seems better to just leave that part as-is for clarity.

Same comment applies to the dialog case, I guess - I imagine it required the executeSoon() since the stuff it's testing depends on the dialog's onload(), which would potentially run after the test's handler?
(In reply to comment #23)
> My point was that we're potentially calling waitForFocus() after the window is
> already focused. What I'd forgotten is that it handles that case by just
> calling the callback immediately... the method name is kind of confusing since
> it doesn't necessarily "wait" for a focus event.

Right.

> Why do we care about focus in that case, though? I seems like it turns out to
> just be a roundabout way of calling executeSoon() like we did before, and so it
> seems better to just leave that part as-is for clarity.

Yes, but executeSoon doesn't guarantee the window being focused.  This actually doesn't seem to have much to do with this particular bug, but it will probably prevent future random oranges.

> Same comment applies to the dialog case, I guess - I imagine it required the
> executeSoon() since the stuff it's testing depends on the dialog's onload(),
> which would potentially run after the test's handler?

Yes, that's right.
Why does the window being focused matter, though? That doesn't seem relevant to any of the tests in the callbacks.
(In reply to comment #25)
> Why does the window being focused matter, though? That doesn't seem relevant to
> any of the tests in the callbacks.

Because we're simulating key presses.

http://enndeakin.wordpress.com/2009/09/08/how-to-fix-focus-related-problems-in-tests/
Oh, I see. That only applies to the dialog load callback though, right? I still don't see a reason to change the |executeSoon(callback);|.
Created attachment 439138 [details] [diff] [review]
Patch (v2)

(In reply to comment #27)
> Oh, I see. That only applies to the dialog load callback though, right? I still
> don't see a reason to change the |executeSoon(callback);|.

After a closer look, I don't think what I said in comment 24 is true, so I took that waitForFocus call out.  Indeed, the test passes without it.
Attachment #438403 - Attachment is obsolete: true
Attachment #439138 - Flags: review?(gavin.sharp)
Attachment #438403 - Flags: review?(gavin.sharp)
Attachment #439138 - Flags: review?(gavin.sharp) → review+
Attachment #438294 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/f0098a349b7a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5
status1.9.1: --- → ?
status1.9.2: --- → ?
This can't land on branches as waitForFocus is not available there.
status1.9.1: ? → ---
status1.9.2: ? → ---
You need to log in before you can comment on or make changes to this bug.