browser_urlbar_search.js doesn't wait for focus/activate events on Linux and can therefore run on a window that's not ready to show the url bar popup yet.

RESOLVED FIXED in Firefox 64

Status

()

defect
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 1 bug, Regressed 1 bug)

Trunk
Firefox 64
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

This uses openNewBrowserWindow - seems reasonable to just make the helper do the waiting, assuming that doesn't break stuff.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fe67b30d8534
wait for focus and activate events when opening browser windows on Linux, r=dao
https://hg.mozilla.org/mozilla-central/rev/fe67b30d8534
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Flags: qe-verify-

Gijs, what code is responsible for focusing the newly created window here? On Ubuntu 18.10, this STR results in the test timing out because the first window opened is never focused:

1. Run vnc4server.
2. export DISPLAY=:1
3. ./mach test toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js

Wasn't the right fix to this bug adding await promiseFocused(...) in the right places?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Ehsan Akhgari from comment #5)

Gijs, what code is responsible for focusing the newly created window here?

Our core code does that. All newly created windows get focus. This was the case prior to this change and prior to bug 1362774 and the other changes in the deps there, but removing the parent-process tab-content about:blank creation broke some of the timing assumptions in various bits of code about when the focus happened, so I made us wait for focus.

On Ubuntu 18.10, this STR results in the test timing out because the first window opened is never focused:

1. Run vnc4server.
2. export DISPLAY=:1

What do these steps do? Why are they necessary? Does it work without them?

  1. ./mach test toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js

Wasn't the right fix to this bug adding `await promiseFocused(...)` in the right places?

No. browser_urlbar_search was just one test that made this assumption, so just fixing it locally wasn't really a serious option. The helper should do the Right Thing, and the only case I'm aware of where this caused serious problems was a plugin test (because plugins and focus interact... strangely), and then only on some platforms / when run manually. See bug 1502226.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #7)

(In reply to :Ehsan Akhgari from comment #5)

Gijs, what code is responsible for focusing the newly created window here?

Our core code does that. All newly created windows get focus.

Umm, which core core? I spent quite a while looking for it without any luck so I'd appreciate if you can point me to it, since I can't find it...

This was the case prior to this change and prior to bug 1362774 and the other changes in the deps there, but removing the parent-process tab-content about:blank creation broke some of the timing assumptions in various bits of code about when the focus happened, so I made us wait for focus.

I see. Could it be that we used to focus windows and we don't any more?

On Ubuntu 18.10, this STR results in the test timing out because the first window opened is never focused:

1. Run vnc4server.
2. export DISPLAY=:1

What do these steps do? Why are they necessary? Does it work without them?

They launch a VNC server and allow me to run the test inside it. (This is a remote machine which I don't have physical access to.) I don't know if it works without them because I don't have physical access to the machine, but in this VNC server I get a very barebones window manager (not Ubuntu's default) which may be why the window focus isn't happening. Clicking on the window to focus it manually makes the problem go away.

  1. ./mach test toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js

Wasn't the right fix to this bug adding `await promiseFocused(...)` in the right places?

No. browser_urlbar_search was just one test that made this assumption, so just fixing it locally wasn't really a serious option. The helper should do the Right Thing, and the only case I'm aware of where this caused serious problems was a plugin test (because plugins and focus interact... strangely), and then only on some platforms / when run manually. See bug 1502226.

OK. I'm still puzzled as to why doing the right thing was to wait for focus and never trigger it...

(In reply to :Ehsan Akhgari from comment #8)

(In reply to :Gijs (he/him) from comment #7)

(In reply to :Ehsan Akhgari from comment #5)

Gijs, what code is responsible for focusing the newly created window here?

Our core code does that. All newly created windows get focus.

Umm, which core core? I spent quite a while looking for it without any luck so I'd appreciate if you can point me to it, since I can't find it...

I have no idea off-hand. Perhaps this is all just a result of frontend code calling .focus() on the URL bar and/or the content browser when the window opens. Newly opened windows are normally activated by widget code which may also cause focus. But I mean, it is observably the case that if you open the browser, something in the initial window gets focus. Ditto for when you open a new window with accel-n. Does that not happen in your VM/VNC thingy? That's where I'd start digging - but in any case, as I said, Neil is the expert here if you have questions, I just ran into this stuff as part of fixing bug 1362774.

This was the case prior to this change and prior to bug 1362774 and the other changes in the deps there, but removing the parent-process tab-content about:blank creation broke some of the timing assumptions in various bits of code about when the focus happened, so I made us wait for focus.

I see. Could it be that we used to focus windows and we don't any more?

Seems unlikely, because then this test (and a great many others) would be failing everywhere, and it's not...

On Ubuntu 18.10, this STR results in the test timing out because the first window opened is never focused:

1. Run vnc4server.
2. export DISPLAY=:1

What do these steps do? Why are they necessary? Does it work without them?

They launch a VNC server and allow me to run the test inside it. (This is a remote machine which I don't have physical access to.) I don't know if it works without them because I don't have physical access to the machine, but in this VNC server I get a very barebones window manager (not Ubuntu's default) which may be why the window focus isn't happening. Clicking on the window to focus it manually makes the problem go away.

But running the test locally works in other environments, no? It's working on infra if it's checked in, presumably...

OK. I'm still puzzled as to why doing the right thing was to wait for focus and never trigger it...

Because something else always triggers focus, and this is the "normal" behavior in a browser window that gets opened, so if I'm writing a test and I want to test some interaction in a new window, this is what is expected.

Not waiting for this "natural" focus is problematic because then it arrives when we're in the middle of attempting to do other things, like opening popups (which will be closed on Linux if there is a window focus/activation change, which in turn breaks tests...). IOW, this is a generic helper that's used by loads of browser tests, and it should DWIM for most people, which normally includes waiting for focus to have settled.

If you've found an edgecase where focus never happens, that's interesting but probably best off in a new bug with more details once you have them.

Attachment #9061573 - Attachment is obsolete: true

(In reply to :Gijs (he/him) from comment #9)

(In reply to :Ehsan Akhgari from comment #8)

(In reply to :Gijs (he/him) from comment #7)

(In reply to :Ehsan Akhgari from comment #5)

Gijs, what code is responsible for focusing the newly created window here?

Our core code does that. All newly created windows get focus.

Umm, which core core? I spent quite a while looking for it without any luck so I'd appreciate if you can point me to it, since I can't find it...

I have no idea off-hand. Perhaps this is all just a result of frontend code calling .focus() on the URL bar and/or the content browser when the window opens. Newly opened windows are normally activated by widget code which may also cause focus. But I mean, it is observably the case that if you open the browser, something in the initial window gets focus.

OK.

Ditto for when you open a new window with accel-n. Does that not happen in your VM/VNC thingy?

No it does not.

That's where I'd start digging - but in any case, as I said, Neil is the expert here if you have questions, I just ran into this stuff as part of fixing bug 1362774.

The reason why I started looking here was that I found out my test is waiting hanging on this event that seems to never be happening and this bug was the one that had added the waits and it wasn't clear what the waits were pending for. Sorry for the hassle, this isn't really worth debugging all of our focus manager code. I'll just switch my test to use the BrowserTestUtils API to avoid the problem.

This was the case prior to this change and prior to bug 1362774 and the other changes in the deps there, but removing the parent-process tab-content about:blank creation broke some of the timing assumptions in various bits of code about when the focus happened, so I made us wait for focus.

I see. Could it be that we used to focus windows and we don't any more?

Seems unlikely, because then this test (and a great many others) would be failing everywhere, and it's not...

Just to be clear, any and all tests using this API are broken right now for me as a result of this change. :-) Thankfully I have to run only one such test on a daily basis.

On Ubuntu 18.10, this STR results in the test timing out because the first window opened is never focused:

1. Run vnc4server.
2. export DISPLAY=:1

What do these steps do? Why are they necessary? Does it work without them?

They launch a VNC server and allow me to run the test inside it. (This is a remote machine which I don't have physical access to.) I don't know if it works without them because I don't have physical access to the machine, but in this VNC server I get a very barebones window manager (not Ubuntu's default) which may be why the window focus isn't happening. Clicking on the window to focus it manually makes the problem go away.

But running the test locally works in other environments, no? It's working on infra if it's checked in, presumably...

Yes of course it does. Repeating myself again, the problem I'm talking about is that these tests break down when the default window manager (gdm?) isn't there to help us focus the window, be it a VNC server, xvfb-run, etc. (I don't know all of the right terminology here... I'll attach a screenshot of the browser window inside VNC viewer to demonstrate what I mean -- the barebones window manager present here for example doesn't give you window borders, or the ability to resize them, etc.)

OK. I'm still puzzled as to why doing the right thing was to wait for focus and never trigger it...

Because something else always triggers focus, and this is the "normal" behavior in a browser window that gets opened, so if I'm writing a test and I want to test some interaction in a new window, this is what is expected.

Not waiting for this "natural" focus is problematic because then it arrives when we're in the middle of attempting to do other things, like opening popups (which will be closed on Linux if there is a window focus/activation change, which in turn breaks tests...). IOW, this is a generic helper that's used by loads of browser tests, and it should DWIM for most people, which normally includes waiting for focus to have settled.

If you've found an edgecase where focus never happens, that's interesting but probably best off in a new bug with more details once you have them.

I'll file a bug about this, I don't think continuing this back and forth on a resolved bug is particularly helpful, since it feels like you're outright rejecting the notion that there is a bug here in the first place.

Regressions: 1548085
You need to log in before you can comment on or make changes to this bug.