541404 - browser_privatebrowsing_openlocation.js hangs when I run it on Ubuntu

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[(not currently active) Ted Mielczarek]

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

Comment 1

[(no longer active)]

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...

Comment 2

[(no longer active)]

Another thing to try:

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

javascript:alert(document.getElementById("dialog.input"));

Comment 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[(no longer active)]

Attachment: 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?

Comment 5

[(not currently active) Ted Mielczarek]

Sorry, forgot about this, building with this patch now.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: log output

Here's the output with that patch.

Comment 7

[(no longer active)]

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?

Comment 8

[(not currently active) Ted Mielczarek]

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.

Comment 9

[(no longer active)]

(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.  :-/

Comment 10

[(not currently active) Ted Mielczarek]

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.

Comment 11

[(no longer active)]

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?

Comment 13

[(no longer active)]

We have a log from one of the Fedora unit test boxes.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270892665.1270893238.22014.gz

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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.

Comment 15

[(no longer active)]

(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?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: log of failures with previous patch

Comment 17

[(no longer active)]

Attachment: Patch

I _think_ this should solve the problem.  dbaron, would you mind testing this please?

Comment 18

[(no longer active)]

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.

Comment 19

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

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?

Comment 20

[(no longer active)]

(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?

Comment 21

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

(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...

Comment 22

[(no longer active)]

(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.

Comment 23

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

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?

Comment 24

[(no longer active)]

(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.

Comment 25

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

Why does the window being focused matter, though? That doesn't seem relevant to any of the tests in the callbacks.

Comment 26

[(no longer active)]

(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/

Comment 27

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

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);|.

Comment 28

[(no longer active)]

Attachment: 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.

Comment 29

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f0098a349b7a

Comment 30

[(no longer active)]

This can't land on branches as waitForFocus is not available there.