1692716 - Make browser_quit_disabled.js more robust

Status: RESOLVED FIXED

(Firefox :: Keyboard Navigation, enhancement)

Description

[Tom S [:evilpie]]

I realized that we wouldn't notice if the quit-application-requested observer wasn't called.

Comment 1

[Tom S [:evilpie]]

Attachment: Bug 1692716 - Make browser_quit_disabled.js more robust. r?Gijs

I realized that we wouldn't notice if the quit-application-requested observer wasn't called.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b17481ba70ba
Make browser_quit_disabled.js more robust. r=Gijs

Comment 3

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures in browser_quit_disabled

Backout link: https://hg.mozilla.org/integration/autoland/rev/ea98b384468e40aba13b07c717d02d88bc0b2f40

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&fromchange=0abd3454600de062e5dca4140a71de592534a8c8&searchStr=OS%2CX%2C10.14%2CWebRender%2Copt%2CMochitests%2Ctest-macosx1014-64-qr%2Fopt-mochitest-browser-chrome-e10s%2Cbc1&tochange=f1e025de3da2e4c3820722ad301f6896f30c135a&selectedTaskRun=HQ2u2lAeSL-Gz3hMHoSJjg.0

Failure log:https://treeherder.mozilla.org/logviewer?job_id=329988007&repo=autoland&lineNumber=6049

"INFO - TEST-START | browser/components/tests/browser/browser_quit_disabled.js
[task 2021-02-15T11:40:27.196Z] 11:40:27 INFO - TEST-INFO | started process screencapture
[task 2021-02-15T11:40:27.247Z] 11:40:27 INFO - TEST-INFO | screencapture: exit 0
[task 2021-02-15T11:40:27.247Z] 11:40:27 INFO - Buffered messages logged at 11:40:26
[task 2021-02-15T11:40:27.247Z] 11:40:27 INFO - Entering test bound test_appMenu_quit_disabled
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - TEST-PASS | browser/components/tests/browser/browser_quit_disabled.js | No quit button with shortcut key -
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Leaving test bound test_appMenu_quit_disabled
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Entering test bound test_quit_shortcut_disabled
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Buffered messages finished
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - TEST-UNEXPECTED-FAIL | browser/components/tests/browser/browser_quit_disabled.js | Expected quit state - Got false, expected true
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Stack trace:
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - chrome://mochikit/content/browser-test.js:test_is:1351
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - chrome://mochitests/content/browser/browser/components/tests/browser/browser_quit_disabled.js:testQuitShortcut:52
[task 2021-02-15T11:40:27.562Z] 11:40:27 INFO - TEST-PASS | browser/components/tests/browser/browser_quit_disabled.js | Expected quit state -
[task 2021-02-15T11:40:27.562Z] 11:40:27 INFO - Leaving test bound test_quit_shortcut_disabled
[task 2021-02-15T11:40:27.562Z] 11:40:27 INFO - GECKO(1852) | MEMORY STAT | vsize 7053MB | residentFast 334MB | heapAllocated 154MB
[task 2021-02-15T11:40:27.563Z] 11:40:27 INFO - TEST-OK | browser/components/tests/browser/browser_quit_disabled.js | took 1405ms
[task 2021-02-15T11:40:27.565Z] 11:40:27 INFO - checking window state
[task 2021-02-15T11:40:27.670Z] 11:40:27 INFO - TEST-START | browser/components/tests/browser/browser_startup_homepage.js"

Comment 4

[Tom S [:evilpie]]

Seems like the test only fails on macOS. I won't be able to debug this. However :arai told me before that the pref worked for them. So this might just be a test problem.

Comment 5

[Tom S [:evilpie]]

Would you mind taking a look? I don't own any Apple hardware.

Comment 6

[:Gijs (he/him)]

(In reply to Tom Schuster [:evilpie] from comment #5)

Would you mind taking a look? I don't own any Apple hardware.

I have a mac, and I can reproduce. I tried replacing the synthesizeKey with synthesizeNativeKey with the associated magic incantations:

    if (AppConstants.platform == "macosx") {
      EventUtils.synthesizeNativeKey({
  name: "US",
  Mac: 0,
  Win: 0x00000409,
  hasAltGrOnWin: false,
}, 0x0c /* MAC_VK_ANSI_Q */, {metaKey: 1}, "q", "q", null, win);
    } else {
      EventUtils.synthesizeKey("q", modifiers, win);
    }

(don't mind the formatting)

The good news is that fixes the issue, the bad news is it breaks the test the other way, ie the quit shortcut "works" when it shouldn't. This is because the code in the hidden window hasn't run, and macOS, when failing to find a workable shortcut in the window against which we run it, tries the no-window shortcuts (this is probably a factor of our cocoa integration, but anyway). So it tries to quit anyway:

Unexpected Results
------------------
browser/components/tests/browser/browser_quit_disabled.js
  FAIL Quit shortcut should NOT have worked - 
Stack trace:
chrome://mochikit/content/browser-test.js:test_ok:1323
chrome://mochitests/content/browser/browser/components/tests/browser/browser_quit_disabled.js:observe:32
chrome://global/content/globalOverlay.js:canQuitApplication:50
chrome://global/content/globalOverlay.js:goQuitApplication:65
chrome://browser/content/hiddenWindowMac.xhtml:oncommand:1
  FAIL Expected quit state - Got true, expected false
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1351
chrome://mochitests/content/browser/browser/components/tests/browser/browser_quit_disabled.js:testQuitShortcut:61

At this point I would suggest re-landing with the test disabled on macOS. Alternatively, you could make the mac hidden window code use a pref observer to enable/disable the shortcut and then things should work...

Comment 7

[Tom S [:evilpie]]

I have to admit I don't really understand the mac hidden window stuff. I've heard of it, but it seems really strange. So does that mean browser.quitShortcut.disabled=true sometimes doesn't disable the shortcut on mac? Or is this a test only problem?

Comment 8

[:Gijs (he/him)]

(In reply to Tom Schuster [:evilpie] from comment #7)

I have to admit I don't really understand the mac hidden window stuff. I've heard of it, but it seems really strange. So does that mean browser.quitShortcut.disabled=true sometimes doesn't disable the shortcut on mac? Or is this a test only problem?

Well, the pref is only checked once for each window. So the new window being created after the pref has changed gets the effect. But the hidden window (which is created early during startup) reads the initial value of the pref, before the test changes it, and thus doesn't have the shortcut removed.

If you made the pref's effect "fully restartless", as it were (so you could toggle it and existing windows would update), then I suspect you could test it. But that's a bunch more work...

Comment 9

[Tom S [:evilpie]]

Thanks for the explanation. Making the pref "fully restartless" would involve having some way to actually going back from disable to enabled. That seems like a bunch of work, as you said, for something that we probably don't need to support for normal usage. I am fine with disabling the test on macOS.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/900e66a5ebae
Make browser_quit_disabled.js more robust. r=Gijs

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/900e66a5ebae