1176281 - Cleanup and taskify referrer tests

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Biru [:poiru]]

I started looking into bug 1145199 with Franziskus and decided to do a little cleanup.

Comment 1

[Biru [:poiru]]

Attachment: Part 1: Get rid of CRLF line-endings in browser/base/content/test/referrer/

Comment 2

[Biru [:poiru]]

Attachment: Part 2: Simplify clickTheLink using BrowserTestUtils.synthesizeMouseAtCenter

Comment 3

[Biru [:poiru]]

Attachment: Part 3: Simplify contextMenuOpened using BrowserTestUtils.waitForEvent

Comment 4

[Biru [:poiru]]

Attachment: Part 4: Simplify newWindowOpened using TestUtils.topicObserved

Comment 5

[Biru [:poiru]]

Attachment: Part 5: Add runTestSteps in preparation for Task based tests

Comment 6

[Biru [:poiru]]

Attachment: Taskify browser_referrer_simple_click.js

Comment 7

[Biru [:poiru]]

Attachment: Taskify browser_referrer_middle_click.js

Comment 8

[Biru [:poiru]]

Attachment: Taskify browser_referrer_open_link_in_tab.js

Comment 9

[Biru [:poiru]]

Attachment: Taskify browser_referrer_open_link_in_window.js

Comment 10

[Biru [:poiru]]

Attachment: Part 10: Taskify browser_referrer_open_link_in_private.js

Comment 11

[Biru [:poiru]]

Attachment: Part 11: Remove now unused functions in head.js

Comment 12

[:Gijs (he/him)]

Comment on attachment 8624768 [details] [diff] [review]
Part 2: Simplify clickTheLink using BrowserTestUtils.synthesizeMouseAtCenter

Review of attachment 8624768 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 13

[:Gijs (he/him)]

Comment on attachment 8624771 [details] [diff] [review]
Part 3: Simplify contextMenuOpened using BrowserTestUtils.waitForEvent

Review of attachment 8624771 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me, though at this point I wonder how much use there is in having this be a utility function. Ditto for the previous patch.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8624774 [details] [diff] [review]
Part 5: Add runTestSteps in preparation for Task based tests

Review of attachment 8624774 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/referrer/head.js
@@ +66,5 @@
>    },
>  ];
>  
> +let runTestSteps = Task.async(function* (checkFn) {
> +  let testWindow = yield BrowserTestUtils.openNewBrowserWindow();

FWIW, it was never clear to me why all these tests need their own new window. You could just run the tests in the pre-existing window, and that would shorten the test run and reduce the opportunities for us to make mistakes resulting in intermittent failures.

Seeing as we're redoing these tests, I'm going to actually ask that we do that unless there's a good reason not to.

@@ +71,5 @@
> +  registerCleanupFunction(() => testWindow.close());
> +
> +  for (let test of _referrerTests) {
> +    let url = test.fromScheme + REFERRER_POLICYSERVER_URL +
> +              "?scheme=" + escape(test.toScheme) +

nit: please use encodeURIComponent instead of escape().

@@ +78,5 @@
> +    let desc = "policy=[" + test.policy + "] " +
> +               "rel=[" + test.rel + "] " +
> +               test.fromScheme + " -> " + test.toScheme;
> +
> +    let tab = yield BrowserTestUtils.openNewForegroundTab(testWindow.gBrowser,

so if we don't need a new window here, you can just pass gBrowser.

@@ +81,5 @@
> +
> +    let tab = yield BrowserTestUtils.openNewForegroundTab(testWindow.gBrowser,
> +                                                          url, true);
> +
> +    let result = yield checkFn(testWindow);

... and |window| here.

However, the functions you're passing to this function in the later patches are generator functions using yield, so ITYM yield*. Right? Or am I missing something?

Comment 15

[:Gijs (he/him)]

Comment on attachment 8624776 [details] [diff] [review]
Taskify browser_referrer_middle_click.js

Review of attachment 8624776 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/referrer/browser_referrer_middle_click.js
@@ +6,2 @@
>  
> +  let tab = yield someTabLoaded(testWindow);

This would race if clickTheLink were synchronous about the new tab, no? Maybe that can never happen and I'm just being paranoid (though I think e10s and the yield and the chained promises in some of the earlier patches create more than one event loop spin here and in the clickTheLink call...), but considering how this was all very intermittent so far, I think maybe a little paranoia might be healthy.

Seems like it would be safer to create this promise before you clickTheLink, and yield afterwards.

r=me with that fixed.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8624775 [details] [diff] [review]
Taskify browser_referrer_simple_click.js

Review of attachment 8624775 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/referrer/browser_referrer_simple_click.js
@@ +6,2 @@
>  
> +  yield BrowserTestUtils.browserLoaded(testWindow.gBrowser.selectedBrowser);

See my earlier remarks. You want to create the promise before you do something that can resolve it, and yield afterwards.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8624778 [details] [diff] [review]
Taskify browser_referrer_open_link_in_tab.js

Review of attachment 8624778 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/referrer/browser_referrer_open_link_in_tab.js
@@ +2,5 @@
>  // Selects "open link in new tab" from the context menu.
>  
> +function* checkStep(testWindow) {
> +  let contextMenu = yield contextMenuOpened(testWindow, "testlink");
> +  doContextMenuCommand(testWindow, contextMenu, "context-openlinkintab");

Having been bitten by stuff like this... what happens if the menu is hidden again by the time this executes? Note that the yield and promise resolve calls introduce an event loop spin here.

It might be safer to integrate the item that needs clicking and the doContextMenuCommand call within the contextMenuOpened handler... and call it synchronously from the event handler. Of course, that requires hacking it into the checkEvent function that you pass into waitForEvent, which is a little evil.

Then, however, you wouldn't actually need to yield here...

@@ +7,2 @@
>  
> +  let tab = yield someTabLoaded(testWindow);

... but could suffice with yielding for this promise, which (broken record alert) you should create before causing the tab to be loaded.

Comment 18

[Biru [:poiru]]

Attachment: Part 5: Add runTestSteps in preparation for Task based tests

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8624774 [details] [diff] [review]
> Part 5: Add runTestSteps in preparation for Task based tests
> 
> Review of attachment 8624774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/referrer/head.js
> @@ +66,5 @@
> >    },
> >  ];
> >  
> > +let runTestSteps = Task.async(function* (checkFn) {
> > +  let testWindow = yield BrowserTestUtils.openNewBrowserWindow();
> 
> FWIW, it was never clear to me why all these tests need their own new
> window. You could just run the tests in the pre-existing window, and that
> would shorten the test run and reduce the opportunities for us to make
> mistakes resulting in intermittent failures.

Done. This occured to me, too, but I figured I'd just follow the existing tests.

> However, the functions you're passing to this function in the later patches
> are generator functions using yield, so ITYM yield*. Right?

Yep, fixed.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8624779 [details] [diff] [review]
Taskify browser_referrer_open_link_in_window.js

Review of attachment 8624779 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/referrer/browser_referrer_open_link_in_window.js
@@ +6,3 @@
>  
> +  let newWindowPromise = newWindowOpened();
> +  doContextMenuCommand(testWindow, contextMenu, "context-openlink");

Same as in the other ctxt menu test I just commented on applies here.

@@ +6,5 @@
>  
> +  let newWindowPromise = newWindowOpened();
> +  doContextMenuCommand(testWindow, contextMenu, "context-openlink");
> +  let newWindow = yield newWindowPromise;
> +  yield someTabLoaded(newWindow);

OK, but this is *definitely* going to race. You're waiting for browser-delayed-startup, which could totally have finished after the tab has loaded, esp. on e10s.

This is kind of rock/hard-place material because it's not currently easy for you to say "wait for a tab load in a window that doesn't exist yet".

It might make more sense to use one of the evil content window creation observer thingies. Although that'll break if we start using more than one content process.

I don't have great answers here... needinfo mconley to see if he has suggestions.

Comment 20

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #19)
> @@ +6,5 @@
> >  
> > +  let newWindowPromise = newWindowOpened();
> > +  doContextMenuCommand(testWindow, contextMenu, "context-openlink");
> > +  let newWindow = yield newWindowPromise;
> > +  yield someTabLoaded(newWindow);
> 
> OK, but this is *definitely* going to race. You're waiting for
> browser-delayed-startup, which could totally have finished after the tab has
> loaded, esp. on e10s.
> 
> This is kind of rock/hard-place material because it's not currently easy for
> you to say "wait for a tab load in a window that doesn't exist yet".
> 
> It might make more sense to use one of the evil content window creation
> observer thingies. Although that'll break if we start using more than one
> content process.
> 
> I don't have great answers here... needinfo mconley to see if he has
> suggestions.

Can't needinfo off splinter, so doing that now...

Comment 21

[:Gijs (he/him)]

Comment on attachment 8624802 [details] [diff] [review]
Part 5: Add runTestSteps in preparation for Task based tests

Review of attachment 8624802 [details] [diff] [review]:
-----------------------------------------------------------------

I'm hoping/assuming you re-ran these locally both with --e10s and without, and they still work? :-)

Comment 22

[:Gijs (he/him)]

Comment on attachment 8624782 [details] [diff] [review]
Part 10: Taskify browser_referrer_open_link_in_private.js

Review of attachment 8624782 [details] [diff] [review]:
-----------------------------------------------------------------

This has the same issues as open_link_in_window.js

Comment 23

[:Gijs (he/him)]

Comment on attachment 8624783 [details] [diff] [review]
Part 11: Remove now unused functions in head.js

Review of attachment 8624783 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 24

[Biru [:poiru]]

Attachment: Part 12: Perform the context menu command within the popupshown event handler

(In reply to :Gijs Kruitbosch from comment #17)
> It might be safer to integrate the item that needs clicking and the
> doContextMenuCommand call within the contextMenuOpened handler... and call
> it synchronously from the event handler. Of course, that requires hacking it
> into the checkEvent function that you pass into waitForEvent, which is a
> little evil.

Done. If you prefer, I can move this after Part 4.

(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8624802 [details] [diff] [review]
> Part 5: Add runTestSteps in preparation for Task based tests
> 
> Review of attachment 8624802 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm hoping/assuming you re-ran these locally both with --e10s and without,
> and they still work? :-)

Yep. I'll test on try as well before landing this.

Comment 25

[Biru [:poiru]]

Attachment: Part 8: Taskify browser_referrer_open_link_in_tab.js

Comment 26

[:Gijs (he/him)]

Comment on attachment 8624824 [details] [diff] [review]
Part 12: Perform the context menu command within the popupshown event handler

Review of attachment 8624824 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 27

[:Gijs (he/him)]

Comment on attachment 8624826 [details] [diff] [review]
Part 8: Taskify browser_referrer_open_link_in_tab.js

Review of attachment 8624826 [details] [diff] [review]:
-----------------------------------------------------------------

this gets changed again by the 12th patch, right? A little confused now, but sure...

Comment 28

[:Gijs (he/him)]

fwiw, still worried over comment #19/20, and not entirely sure what to suggest. You could make the page you open send a message that you listen for in a framescript that you ensure loads for every tab. I forget if you can use contenttask for that or if that's specific to a single <browser> (I think the latter).

Comment 29

[Biru [:poiru]]

(In reply to :Gijs Kruitbosch from comment #27)
> Comment on attachment 8624826 [details] [diff] [review]
> Part 8: Taskify browser_referrer_open_link_in_tab.js
> 
> Review of attachment 8624826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this gets changed again by the 12th patch, right? A little confused now, but
> sure...

Yes, sorry. I meant to re-request the cancelled review, but ended up resubmitting the patch.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8624779 [details] [diff] [review]
Taskify browser_referrer_open_link_in_window.js

Review of attachment 8624779 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/referrer/browser_referrer_open_link_in_window.js
@@ +6,5 @@
>  
> +  let newWindowPromise = newWindowOpened();
> +  doContextMenuCommand(testWindow, contextMenu, "context-openlink");
> +  let newWindow = yield newWindowPromise;
> +  yield someTabLoaded(newWindow);

You might consider using ContentTask.spawn and document.readyState - example:

let browser = newWindow.gBrowser.selectedBrowser;
yield ContentTask.spawn(browser, null, function*() {
  yield new Promise((resolve) => {
    let doc = content.document;
    if (doc.readyState == "loaded") {
      // All done before we got here.
      return resolve();
    }

    doc.addEventListener("readystatechange", function onRSC() {
      if (doc.readyState == "loaded") {
        doc.removeEventListener("readystatechange", onRSC);
        resolve();
      }
    });
  });
});

<the rest of your test>

I _think_ that should be resilient against the myriad of states that the document might be at this time.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3cb074c421f
https://hg.mozilla.org/integration/mozilla-inbound/rev/af9a6eb0b4e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/751ca11fdcfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/565bb767379d

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3cb074c421f
https://hg.mozilla.org/mozilla-central/rev/af9a6eb0b4e3
https://hg.mozilla.org/mozilla-central/rev/751ca11fdcfe
https://hg.mozilla.org/mozilla-central/rev/565bb767379d

Comment 33

[Biru [:poiru]]

Attachment: Part 13: Fix racy checks for tab/window loads

Comment 34

[Biru [:poiru]]

Comment on attachment 8632132 [details] [diff] [review]
Part 13: Fix racy checks for tab/window loads

Turns out this doesn't actually fix anything.

Comment 35

[Biru [:poiru]]

I sadly don't have time to look into this at the moment.

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:Dolske, maybe it's time to close this bug?

Comment 37

[:Gijs (he/him)]

We can call this fixed and do any other refactoring in a separate bug.