1246664 - Enable browser_f7_caret_browsing.js in e10s

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Neil Deakin]

Attachment: browser_f7_caret_browsing.js

Comment 1

[Neil Deakin]

This fails intermittently with this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d84120d7d9a4

Comment 2

[Justin Dolske [:Dolske]]

Neil: Did you have any further info/thoughts on what was failing here? I assume you unassigned yourself because you're unable to work on it?

Gijs: Might you be able to pick this up? I think this is actually a test you wrote. ;-) And it's one of the last handful of front-end E10S tests still disabled.

Comment 3

[Neil Deakin]

I don't think I ever made an attempt to look at what was wrong.

Comment 4

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #2)
> Gijs: Might you be able to pick this up? I think this is actually a test you
> wrote. ;-) And it's one of the last handful of front-end E10S tests still
> disabled.

Yes, I can have a look.

Comment 5

[:Gijs (he/him)]

I can't reproduce the failures locally but I noticed a different issue which I think may be a result of the same problem. The patch adjusted the test to use BTU.domWindowOpened inside promiseCaretPromptOpened. I'm a big fan of using BTU in general, but it has one downside: you can't "undo" waiting for that promise, and remove items from a promise's .then() list. So I got errors at the end of the test where BrowserTestUtils in the .then() from the domWindowOpened (which then waits for a load event) was not defined.

I expect that it's also possible, given the right timing, for the listener to fire on a later load of the window and actually end up calling the callback that does a win.close() in syncToggleCaretNoDialog. Which would explain why things get out of whack, maybe.

So I re-added some of the "manual" listening code, and hopefully that will help?

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdac97ce1d25


I will doubtless have to retrigger a bunch tomorrow.

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> I can't reproduce the failures locally but I noticed a different issue which
> I think may be a result of the same problem. The patch adjusted the test to
> use BTU.domWindowOpened inside promiseCaretPromptOpened. I'm a big fan of
> using BTU in general, but it has one downside: you can't "undo" waiting for
> that promise, and remove items from a promise's .then() list. So I got
> errors at the end of the test where BrowserTestUtils in the .then() from the
> domWindowOpened (which then waits for a load event) was not defined.
> 
> I expect that it's also possible, given the right timing, for the listener
> to fire on a later load of the window and actually end up calling the
> callback that does a win.close() in syncToggleCaretNoDialog. Which would
> explain why things get out of whack, maybe.
> 
> So I re-added some of the "manual" listening code, and hopefully that will
> help?
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdac97ce1d25
> 
> 
> I will doubtless have to retrigger a bunch tomorrow.

That failure seems gone, but we have a new thing. I don't really know what's happening there yet... can't reproduce on my own win8 machine. New try run:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9159d0625b59

Comment 7

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1246664 - enable browser_f7_caret_browsing.js in e10s, r?mikedeboer

Review commit: https://reviewboard.mozilla.org/r/52766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52766/

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8752751 [details]
MozReview Request: Bug 1246664 - enable browser_f7_caret_browsing.js in e10s, r?mikedeboer

https://reviewboard.mozilla.org/r/52766/#review50102

::: toolkit/content/tests/browser/browser_f7_caret_browsing.js:27
(Diff revision 1)
>  
> -function waitForCondition(aConditionFn, aMaxTries=50, aCheckInterval=100) {
> -  function tryNow() {
> -    tries++;
> -    if (aConditionFn()) {
> -      deferred.resolve();
> +// NB: not using BrowserTestUtils.domWindowOpened here because there's no way to
> +// undo waiting for a window open. If we don't want the window to be opened, and
> +// wait for it to verify that it indeed does not open, we need to be able to
> +// then "stop" waiting so that when we next *do* want it to open, our "old"
> +// listener doesn't fire and do things we don't want (like close the window...).

Thanks for this explanation!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/383429412949

Comment 10

[:Gijs (he/him)]

Annnnd backed out by sheriffs for the same orange that we saw originally that then went away in the trypush in comment 7, because of course...:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc56d9b9fa4

Comment 11

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e984434077

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/029dcd150347

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/029dcd150347