Closed Bug 1336335 Opened 5 years ago Closed 5 years ago

Intermittent browser/base/content/test/general/browser_invalid_uri_back_forward_manipulation.js | Test timed out -

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: Gijs)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed])

Attachments

(2 files)

:Gijs - Can you make your new test more reliable?
Blocks: 1226781
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Geoff Brown [:gbrown] from comment #4)
> :Gijs - Can you make your new test more reliable?

No, this isn't a new test - it's a year old, hasn't been changed since Jan. 2016 according to hg blame. What's caused this to suddenly intermittently go orange?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gbrown)
Sorry! I saw "2016-02-02" in the hg log and thought that matched up with the start of this bug -- off by a year!

In that case, I'm puzzled too...what changed?
Flags: needinfo?(gbrown)
This test is going crazy. Joel, any chance you can help track down what caused the increase in frequency? I'm really short on cycles myself, sorry. Looks like 1/10 to 1/5 runs are failing at this point.
Flags: needinfo?(jmaher)
(In reply to :Gijs from comment #8)
> This test is going crazy. Joel, any chance you can help track down what
> caused the increase in frequency? I'm really short on cycles myself, sorry.
> Looks like 1/10 to 1/5 runs are failing at this point.

Oh, I guess that number is per-push, but it's almost entirely linux, so... still probably 1/15-1/20 ish per linux platform
it looks like we see failures really spike on Feb 13th, but possibly on the 12th based on the graph, let me do a bunch of retriggers :)
Flags: needinfo?(jmaher)
and of course, we are spending a lot of time on a non-e10s test :(
I narrowed it down to this range:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=linux%20opt%20mochitests%20executed%20by%20taskcluster%20test-linux32%2Fopt-mochitest-browser-chrome-&tochange=7002008dc1166b7bc1c2ad2ecbcfdde17f6f11b0&fromchange=8a2f028e6943d68259be6e3082f07acd67c3b085

and suspect bug 1304306 (which is where we change from bc4 -> bc3)- although run-by-dir should isolate any profile browser session issues.

Keep in mind not all oranges are the exact failure

:Gijs, mind weighing in on your thoughts given that smaller range of commits?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Joel Maher ( :jmaher) from comment #15)
> I narrowed it down to this range:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-
> searchStr=linux%20opt%20mochitests%20executed%20by%20taskcluster%20test-
> linux32%2Fopt-mochitest-browser-chrome-
> &tochange=7002008dc1166b7bc1c2ad2ecbcfdde17f6f11b0&fromchange=8a2f028e6943d68
> 259be6e3082f07acd67c3b085
> 
> and suspect bug 1304306 (which is where we change from bc4 -> bc3)- although
> run-by-dir should isolate any profile browser session issues.
> 
> Keep in mind not all oranges are the exact failure
> 
> :Gijs, mind weighing in on your thoughts given that smaller range of commits?

Sorry for the delay. I don't see anything in here off-hand. But looking at the test, I suspect it just needs to use BTU's new util to wait for an error page load rather than the hacky hand-rolled approach in the test (which predates the BTU addition). I would write the patch myself but I really don't have cycles right now. See bug 1334455 for similar patches. That might be enough to fix this?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8839198 [details]
Bug 1336335 - fix intermittent failure in browser_invalid_uri_back_forward_manipulation.js by using proper util to wait for an error page load,

https://reviewboard.mozilla.org/r/113902/#review115586

Great, thanks. We might want to solve bug 1334455 (nice bug id) rather sooner than later.
Attachment #8839198 - Flags: review?(jhofmann) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2dfdb7695004
fix intermittent failure in browser_invalid_uri_back_forward_manipulation.js by using proper util to wait for an error page load, r=johannh
Let's leave this open until we confirm this fixes things.
Keywords: leave-open
Looks like https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=afe3edac6acd6ee4c3832226c839ae9f4b3bcce7 contains this fix and still broke. We'll have to wait to see if the frequency is down after this change, but it looks like there's more work to do, anyway.
Not sure this patch will work, but judging from the screenshot and failure message we successfully load about:robots, alter the URL bar value, focus it, and then try to get a load to happen, but for some reason that fails - I suspect there's some kind of race where the synthesized "enter" keypress isn't arriving. As that bit isn't what we're testing here, I'm just forcing a handleCommand invocation instead, which has the same effect for me locally. Trypush to check this helps is underway: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2053d93a5b9f
Hm, have you considered waiting for urlbar focus with BTU.waitForEvent? The urlbar in the screenshot is focused but that could have happened after the keydown before the screenshot. I remember urlbar focus was just really racy for me in bug 1327946, too. There I solved it using listening for focus and synthesizing Ctrl + L.

It's a solution I would slightly prefer to handleCommand, although I can imagine that also works fine. What do you think?
(In reply to Johann Hofmann [:johannh] from comment #27)
> Hm, have you considered waiting for urlbar focus with BTU.waitForEvent? The
> urlbar in the screenshot is focused but that could have happened after the
> keydown before the screenshot. I remember urlbar focus was just really racy
> for me in bug 1327946, too. There I solved it using listening for focus and
> synthesizing Ctrl + L.

This is another possibility we could look at...

> It's a solution I would slightly prefer to handleCommand, although I can
> imagine that also works fine. What do you think?

The difference with bug 1327946 is that this test isn't *about* focus, just about loading a URL via the URL bar, and using synthesized keys just generally feels more likely to break for unrelated reasons and/or cause more/other intermittents. Up to you, though, you're reviewing. :-)
Flags: needinfo?(jhofmann)
Comment on attachment 8841965 [details]
Bug 1336335 - avoid potential focus issues by triggering the load of the error page explicitly,

https://reviewboard.mozilla.org/r/116024/#review117728

> The difference with bug 1327946 is that this test isn't *about* focus, just about loading a URL via the URL bar, and using synthesized keys just generally feels more likely to break for unrelated reasons and/or cause more/other intermittents. Up to you, though, you're reviewing. :-)

Good point, let's not make this too complicated. r=me

Thanks!
Attachment #8841965 - Flags: review?(jhofmann) → review+
Flags: needinfo?(jhofmann)
Comment on attachment 8841965 [details]
Bug 1336335 - avoid potential focus issues by triggering the load of the error page explicitly,

https://reviewboard.mozilla.org/r/116024/#review117822

::: browser/base/content/test/general/browser_invalid_uri_back_forward_manipulation.js:16
(Diff revision 1)
>    yield pushPrefs(["keyword.enabled", false]);
>    let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:robots", true);
> +  info("Loaded about:robots");
> +
>    gURLBar.value = "::2600";
>    gURLBar.focus();

Oh, I forgot: This focus call doesn't have any purpose anymore, right?
Comment on attachment 8841965 [details]
Bug 1336335 - avoid potential focus issues by triggering the load of the error page explicitly,

https://reviewboard.mozilla.org/r/116024/#review117824

::: browser/base/content/test/general/browser_invalid_uri_back_forward_manipulation.js:16
(Diff revision 1)
>    yield pushPrefs(["keyword.enabled", false]);
>    let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "about:robots", true);
> +  info("Loaded about:robots");
> +
>    gURLBar.value = "::2600";
>    gURLBar.focus();

An excellent point. Thanks!
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3399e550a30c
avoid potential focus issues by triggering the load of the error page explicitly, r=johannh
Whiteboard: [stockwell fixed]
Judging by OF, this is now fixed. \o/
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1334455
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.