Cleanup and taskify referrer tests

NEW
Unassigned

Status

()

3 years ago
3 years ago

People

(Reporter: poiru, Unassigned)

Tracking

(Blocks: 1 bug, {leave-open})

Trunk
leave-open
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected)

Details

Attachments

(12 attachments, 3 obsolete attachments)

33.67 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
3.02 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.77 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.65 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.53 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.62 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.92 KB, patch
Details | Diff | Splinter Review
1.99 KB, patch
Details | Diff | Splinter Review
6.95 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.57 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
4.96 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.86 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
I started looking into bug 1145199 with Franziskus and decided to do a little cleanup.
(Reporter)

Comment 1

3 years ago
Created attachment 8624765 [details] [diff] [review]
Part 1: Get rid of CRLF line-endings in browser/base/content/test/referrer/
Attachment #8624765 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 2

3 years ago
Created attachment 8624768 [details] [diff] [review]
Part 2: Simplify clickTheLink using BrowserTestUtils.synthesizeMouseAtCenter
Attachment #8624768 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 3

3 years ago
Created attachment 8624771 [details] [diff] [review]
Part 3: Simplify contextMenuOpened using BrowserTestUtils.waitForEvent
Attachment #8624771 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 4

3 years ago
Created attachment 8624772 [details] [diff] [review]
Part 4: Simplify newWindowOpened using TestUtils.topicObserved
Attachment #8624772 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 5

3 years ago
Created attachment 8624774 [details] [diff] [review]
Part 5: Add runTestSteps in preparation for Task based tests
Attachment #8624774 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 6

3 years ago
Created attachment 8624775 [details] [diff] [review]
Taskify browser_referrer_simple_click.js
Attachment #8624775 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 7

3 years ago
Created attachment 8624776 [details] [diff] [review]
Taskify browser_referrer_middle_click.js
Attachment #8624776 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 8

3 years ago
Created attachment 8624778 [details] [diff] [review]
Taskify browser_referrer_open_link_in_tab.js
Attachment #8624778 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 9

3 years ago
Created attachment 8624779 [details] [diff] [review]
Taskify browser_referrer_open_link_in_window.js
Attachment #8624779 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 10

3 years ago
Created attachment 8624782 [details] [diff] [review]
Part 10: Taskify browser_referrer_open_link_in_private.js
Attachment #8624782 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 11

3 years ago
Created attachment 8624783 [details] [diff] [review]
Part 11: Remove now unused functions in head.js
Attachment #8624783 - Flags: review?(gijskruitbosch+bugs)

Updated

3 years ago
Attachment #8624765 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

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

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

rs=me
Attachment #8624768 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 13

3 years ago
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.
Attachment #8624771 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

3 years ago
Attachment #8624772 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 14

3 years ago
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?
Attachment #8624774 - Flags: review?(gijskruitbosch+bugs)

Comment 15

3 years ago
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.
Attachment #8624776 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 16

3 years ago
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.
Attachment #8624775 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 17

3 years ago
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.
Attachment #8624778 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 18

3 years ago
Created attachment 8624802 [details] [diff] [review]
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.
Attachment #8624774 - Attachment is obsolete: true
Attachment #8624802 - Flags: review?(gijskruitbosch+bugs)

Comment 19

3 years ago
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.
Attachment #8624779 - Flags: review?(gijskruitbosch+bugs)

Comment 20

3 years ago
(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...
Flags: needinfo?(mconley)

Comment 21

3 years ago
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? :-)
Attachment #8624802 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 22

3 years ago
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
Attachment #8624782 - Flags: review?(gijskruitbosch+bugs)

Comment 23

3 years ago
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
Attachment #8624783 - Flags: review?(gijskruitbosch+bugs) → review+
(Reporter)

Comment 24

3 years ago
Created attachment 8624824 [details] [diff] [review]
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.
(Reporter)

Updated

3 years ago
Attachment #8624824 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 25

3 years ago
Created attachment 8624826 [details] [diff] [review]
Part 8: Taskify browser_referrer_open_link_in_tab.js
Attachment #8624826 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Updated

3 years ago
Attachment #8624778 - Attachment is obsolete: true

Comment 26

3 years ago
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!
Attachment #8624824 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 27

3 years ago
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...
Attachment #8624826 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 28

3 years ago
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).
(Reporter)

Comment 29

3 years ago
(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 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.
Flags: needinfo?(mconley)
(Reporter)

Updated

3 years ago
Keywords: leave-open
(Reporter)

Comment 33

3 years ago
Created attachment 8632132 [details] [diff] [review]
Part 13: Fix racy checks for tab/window loads
Attachment #8632132 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Updated

3 years ago
Blocks: 1144816
(Reporter)

Comment 34

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

Turns out this doesn't actually fix anything.
Attachment #8632132 - Attachment is obsolete: true
Attachment #8632132 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 35

3 years ago
I sadly don't have time to look into this at the moment.
Assignee: birunthan → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.