[Tracking Requested - why for this release]:

Central as beta simulation : https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=0d9e35367944d97278d6d8b925e733e1f1c245be&selectedJob=228396527

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228396527&repo=try&lineNumber=1442

06:07:15 INFO - TEST-INFO | started process screencapture
06:07:16 INFO - TEST-INFO | screencapture: exit 0
06:07:16 INFO - Buffered messages logged at 06:06:30
06:07:16 INFO - Entering test bound setPref
06:07:16 INFO - Leaving test bound setPref
06:07:16 INFO - Entering test bound testTabStopsNoPage
06:07:16 INFO - Buffered messages logged at 06:06:31
06:07:16 INFO - TEST-PASS | browser/base/content/test/keyboard/browser_toolbarKeyNav.js | URL bar focused for start of test -
06:07:16 INFO - Console message: [JavaScript Error: "getScreenshot(https://example.com/) failed: TypeError: NetworkError when attempting to fetch resource." {file: "resource://activity-stream/lib/Screenshots.jsm" line: 45}]
06:07:16 INFO - getScreenshotForURL@resource://activity-stream/lib/Screenshots.jsm:45:10
06:07:16 INFO -
06:07:16 INFO - Console message: [JavaScript Error: "Unknown collection "main/tippytop"" {file: "resource://services-settings/RemoteSettingsClient.jsm" line: 259}]
06:07:16 INFO - sync@resource://services-settings/RemoteSettingsClient.jsm:259:13
06:07:16 INFO -
06:07:16 INFO - Buffered messages finished
06:07:16 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/keyboard/browser_toolbarKeyNav.js | Test timed out -
06:07:16 INFO - GECKO(926) | MEMORY STAT | vsize 4413MB | residentFast 315MB | heapAllocated 94MB
06:07:16 INFO - TEST-OK | browser/base/content/test/keyboard/browser_toolbarKeyNav.js | took 45018ms
06:07:16 INFO - Not taking screenshot here: see the one that was previously logged
06:07:16 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/keyboard/browser_toolbarKeyNav.js | Found a tab after previous test timed out: about:blank -
06:07:16 INFO - checking window state
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 930
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 935
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 932
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 931
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 934
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 933
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 936
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 927
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 928
06:07:17 INFO - GECKO(926) | Completed ShutdownLeaks collections in process 926
06:07:17 INFO - TEST-START | Shutdown

Jamie, can you look into this?

I'm trying to reproduce this locally, but I can't get it to build. I applied the patch from:
https://hg.mozilla.org/try/rev/0d9e35367944d97278d6d8b925e733e1f1c245be
to central. However, I get build failures in JS:

js/src
In file included from c:/Users/jamie/src/gecko/js/src/builtin/RegExp.cpp:22:
In file included from c:/Users/jamie/src/gecko/js/src\vm/EnvironmentObject-inl.h:10:
In file included from c:/Users/jamie/src/gecko/js/src\vm/EnvironmentObject.h:10:
In file included from c:/Users/jamie/src/gecko/js/src\builtin/ModuleObject.h:15:
In file included from c:/Users/jamie/src/gecko/js/src\frontend/EitherParser.h:24:
In file included from c:/Users/jamie/src/gecko/js/src\frontend/BCEParserHandle.h:12:
c:/Users/jamie/src/gecko/js/src\frontend/Parser.h(272,29):  error: out-of-line declaration of 'TraceBinParser' does not match any declaration in namespace 'js::frontend'; did you mean 'TraceParser'?
  friend void js::frontend::TraceBinParser(JSTracer* trc,
                            ^~~~~~~~~~~~~~
                            TraceParser
c:/Users/jamie/src/gecko/js/src\frontend/BytecodeCompiler.h(118,6):  note: 'TraceParser' declared here
void TraceParser(JSTracer* trc, JS::AutoGCRooter* parser);
     ^
1 error generated.

Is there some other patch I need to apply?

Hi Jamie, please apply the patches from https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=84d36bfb378e090bcdf060490d21a6b62b4abe6e to get working builds. That's tested with https://hg.mozilla.org/mozilla-central/rev/d97cc5b9eeae57f6fb5d4b595ca68a569afa7bd2 as base.

I can't reproduce this test failure applying those patches locally. I wonder if this is somehow related to having the official branding, which I'm guessing you only get if you build on CI.

I think the problem might be whether the Reload button is enabled or disabled when no page (about:blank) is loaded. Testing manually, this seems to be somewhat quirky. The first time I load about:blank in a new window, the Reload button is disabled. If I close that tab and open a new one with about:blank, the Reload button is enabled. If I press Reload, the Reload button gets disabled and the Back button gets enabled. What is the rule supposed to be regarding about:blank and the Reload button? The key nav tests depend on this being consistent.

(In reply to James Teh [:Jamie] from comment #5)

What is the rule supposed to be regarding about:blank and the Reload button?

https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/browser/base/content/browser.js#4818-4824

I saw that code, but that doesn't really explain this:

The first time I load about:blank in a new window, the Reload button is disabled. If I close that tab and open a new one with about:blank, the Reload button is enabled. If I press Reload, the Reload button gets disabled and the Back button gets enabled.

Surely this inconsistency isn't "intentional"? Is there some way I can make it consistent at least for the purposes of this test?

No, it isn't intentional, it would be a bug in that code. Short of fixing that bug, you could probably also use a custom test page (even just using data:) instead of about:blank.

Please be aware that the merges from central to beta start next Monday and this test will permafail. Thank you.

For a blank tab, the Reload button should be disabled.
These tests depend on this.
This seems to be true when setting the new tab page to blank in Firefox Options.
However, when we open about:blank with BrowserTestUtils.withNewTab, this is unreliable.
That is, sometimes the Reload button is enabled, sometimes it isn't.
I don't understand why this happens.
For the purposes of these tests, just force the Reload button to be disabled for new, blank tabs so we get consistent results.

(In reply to Dão Gottwald [::dao] from comment #8)

No, it isn't intentional, it would be a bug in that code. Short of fixing that bug, you could probably also use a custom test page (even just using data:) instead of about:blank.

These tests are explicitly testing the behaviour when no page is loaded; i.e. the page actions buttons and Back, Forward and Reload buttons should be disabled. So, I can't use a custom page. Hopefully, the hack I've implemented in the tests to work around this will suffice.

https://hg.mozilla.org/mozilla-central/rev/c26d4a8d43c4

This issue is still reoccuring: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f07e637209c105feec8e582c3b55b69f178de4f&searchStr=browser-chrome&group_state=expanded&selectedJob=232764689

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=232764689&repo=try&lineNumber=2625

:jamie any chance you could take a look?

[Tracking Requested - why for this release]:

This fails because the DevEdition has the DevTools button in the toolbar which gets focused instead of the home button as the screenshot artifact ("mozilla-test-fail-screenshot_irul1s.png") of the log shows: https://taskcluster-artifacts.net/Ymkd2aPNQkON6PjN9adaHw/0/public/test_info/mozilla-test-fail-screenshot_irul1s.png

Questions:
Is it expected that the test reports "Test timed out" or should the button mismatch be shown as a human-readable string("Expected X, got Y")?
Should the previous changes in this bug stay in the tree?

Patch is untested, will get added to 2019-03-11 beta simulation.

(In reply to James Teh [:Jamie] from comment #11)

(In reply to Dão Gottwald [::dao] from comment #8)

No, it isn't intentional, it would be a bug in that code. Short of fixing that bug, you could probably also use a custom test page (even just using data:) instead of about:blank.

These tests are explicitly testing the behaviour when no page is loaded; i.e. the page actions buttons and Back, Forward and Reload buttons should be disabled. So, I can't use a custom page. Hopefully, the hack I've implemented in the tests to work around this will suffice.

Jamie, can you check if the button in question that was sometimes disabled and sometimes wasn't is really the reload/stop button (see Aryx's commit about another button interfering on devedition) and if so, file a follow-up bug for that behavior, ideally with concrete steps to reproduce)? Thanks.

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

(In reply to James Teh [:Jamie] from comment #11)

(In reply to Dão Gottwald [::dao] from comment #8)

No, it isn't intentional, it would be a bug in that code. Short of fixing that bug, you could probably also use a custom test page (even just using data:) instead of about:blank.

These tests are explicitly testing the behaviour when no page is loaded; i.e. the page actions buttons and Back, Forward and Reload buttons should be disabled. So, I can't use a custom page. Hopefully, the hack I've implemented in the tests to work around this will suffice.

Jamie, can you check if the button in question that was sometimes disabled and sometimes wasn't is really the reload/stop button (see Aryx's commit about another button interfering on devedition) and if so, file a follow-up bug for that behavior, ideally with concrete steps to reproduce)? Thanks.

Oh, it seems this already happened and is bug 1533633.

https://hg.mozilla.org/mozilla-central/rev/f21b07ac7fb7

It's still failing: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=932e020b1987edc382cb5401e391214ea93d8ab5&selectedJob=233388342

The developer tools button is still present for the failure on beta: https://treeherder.mozilla.org/logviewer.html#?job_id=233916718&repo=mozilla-beta / https://taskcluster-artifacts.net/d8XlIq4eSoeeon0Zv3xfyQ/0/public/test_info//mozilla-test-fail-screenshot_gPgsvA.png

Gijs, can you take a look at the code for the removal of the button and the toolbar reset later, please?

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #24)

The developer tools button is still present for the failure on beta: https://treeherder.mozilla.org/logviewer.html#?job_id=233916718&repo=mozilla-beta / https://taskcluster-artifacts.net/d8XlIq4eSoeeon0Zv3xfyQ/0/public/test_info//mozilla-test-fail-screenshot_gPgsvA.png

Gijs, can you take a look at the code for the removal of the button and the toolbar reset later, please?

The issue is that there's a CUI.reset() in the test already and that re-adds the button. Looking at how best to fix.

https://hg.mozilla.org/mozilla-central/rev/8c0fb16248ee

Verified fixed with today's beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=88ffa9306065a962a3acd1d567aa97e03ac1eb5b

Thanks Gijs and :aryx for fixing this. I'm sorry I was so slow to respond; I was away at a conference last week.