1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux

Status: RESOLVED FIXED

(Firefox :: Security, defect, P1)

Description

[:Paolo Amadini]

The patch for bug 1179961 adds an extra popup action to the Mixed Content Blocking test function "assertMixedContentBlockingState" in the head.js file. This breaks the browser_bug906190.js test on Linux only.

The fix requires making the action asynchronous, but that implies reworking the  Mixed Content Blocking tests to be asynchronous as well. This can be done using Task.jsm. Once this is done, browser_bug906190.js can be re-enabled on Linux too.

Comment 1

[:Paolo Amadini]

There's also bug 1093642 to make this compatible with e10s. That would also be easier if the test is converted to use Tasks.

Comment 2

[Tanvi Vyas[:tanvi]]

I don't feel very comfortable about disabling browser_bug906190.js on Linux.  We should fix this test ASAP and re-enable it.

Comment 3

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

Bug 1212520 - Rewrite Mixed Content Blocking browser-chrome tests with Tasks and re-enable browser_bug906190.js on Linux. r=bgrins

Comment 4

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/23651/#review21149

This is an intermediate state where the test cases are all Tasks but I've not removed most of the repetition yet.

I'm not sure looking at this will be useful for the review, but having it published in MozReview doesn't hurt.

Comment 5

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/23649/#review21175

This version fixes the Linux failure by adding the required wait for the panel to be closed.

I still wnat to do some more cleanup, posting this as an intermediate step. It might be easier to look at the result rather than the diff:

https://reviewboard-hg.mozilla.org/gecko/file/7ea5a32ff20a/browser/base/content/test/general/browser_bug906190.js

Comment 6

[:Paolo Amadini]

Comment on attachment 8680605 [details]
MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

Bug 1212520 - Rewrite Mixed Content Blocking browser-chrome tests with Tasks and re-enable browser_bug906190.js on Linux. r=bgrins

Comment 7

[:Paolo Amadini]

Comment on attachment 8680605 [details]
MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

Tanvi, this is the direction I'm following with the refactoring. Apart from removing the obvious duplication, I've removed some indirection for which the test put some links in content only to retrieve their href later (no simulated clicks on them). This should simplify moving the test to e10s as well.

I'll look into doing more cleanup to use the new BrowserTestUtils helpers where possible.

Comment 8

[:Paolo Amadini]

Comment on attachment 8680605 [details]
MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=bgrins

Comment 9

[:Paolo Amadini]

Note that I haven't been able to make assertMixedContentBlockingState properly asynchronous yet, we still keep the synchronous behavior in all the other tests. When we rewrite all the tests that use it to be asynchronous, we will be able to wait using promisePopupShown before checking the assertions, and then wait using promisePopupHidden before continuing.

Comment 10

[:Paolo Amadini]

Also, it's best to look at the old and new versions of the main test file separately:

https://reviewboard.mozilla.org/r/23651/diff/3/download/414393/orig/
https://reviewboard.mozilla.org/r/23651/diff/3/download/414393/new/

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #9)
> Note that I haven't been able to make assertMixedContentBlockingState
> properly asynchronous yet, we still keep the synchronous behavior in all the
> other tests. When we rewrite all the tests that use it to be asynchronous,
> we will be able to wait using promisePopupShown before checking the
> assertions, and then wait using promisePopupHidden before continuing.

Is that new asynchronous return (the executeSoon) the bit that fixes this in Linux?

Comment 12

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Is that new asynchronous return (the executeSoon) the bit that fixes this in
> Linux?

Apparently so, with this being a "good enough" solution. Ideally this would have been done as per comment 9 by changing assertMixedContentBlockingState to be a function*, but this would have meant adapting all the other tests as well (for which we may want a bug on file).

Comment 13

[:Paolo Amadini]

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

Comment 14

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/23651/#review21307

This looks really nice Paolo!  Great work!  I have reviewed through test 3 and will have to get back to test 4, 5, and 6 tonight.

::: browser/base/content/test/general/browser_bug906190.js:178
(Diff revision 3)
> +  yield doTest(gHttpTestRoot2 + "file_bug906190_1.html",

I think these should both be gHttpTestRoot2 for both of these because you are clicking a child link to the same origin.

Comment 15

[:Paolo Amadini]

(In reply to Tanvi Vyas [:tanvi] from comment #14)
> ::: browser/base/content/test/general/browser_bug906190.js:178
> (Diff revision 3)
> > +  yield doTest(gHttpTestRoot2 + "file_bug906190_1.html",
> 
> I think these should both be gHttpTestRoot2 for both of these because you
> are clicking a child link to the same origin.

I think you're talking about test 4 (now called test_same_origin_metarefresh_different_origin). You're right, the original test didn't do what the description in the comment said. I think the same goes for test 6 (now called test_same_origin_302redirect_different_origin), apparently both links in test 6 should use gHttpTestRoot2 as well.

I've updated the two test cases and they still pass like they did before. I'll include the changes in the next revision of the patch.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8680605 [details]
MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

https://reviewboard.mozilla.org/r/23651/#review21391

I'm going to let Tanvi finish her review of the changes to the test since she knows the details of the test better.  I have some comments regarding the change to assertMixedContentBlockingState below

::: browser/base/content/test/general/head.js:909
(Diff revision 3)
> +  return new Promise(resolve => executeSoon(resolve));

OK, so for this change:

1) Do we definitely need it?  Other tests seem OK with using it synchrounously, so maybe your other add_task fixes will make this test work without this change.
2) If yes to 1, then can we wait for promisePopupHidden instead of executeSoon?  As per Comment 12 since other tests aren't yielding on this function hopefully that wouldn't cause problems.
3) If yes to 1, can you file a bug to convert all calls to assertMixedContentBlockingState to be asynchronous and add that Bug # in the header comment for this function?

Comment 17

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> 2) If yes to 1, then can we wait for promisePopupHidden instead of
> executeSoon?  As per Comment 12 since other tests aren't yielding on this
> function hopefully that wouldn't cause problems.

Yes to 1, and we cannot use promisePopupHidden because it's unreliable unless we use promisePopupShown before simulating the click, and we cannot do that because of the synchronous callers.

> 3) If yes to 1, can you file a bug to convert all calls to
> assertMixedContentBlockingState to be asynchronous and add that Bug # in the
> header comment for this function?

Filed bug 1221114, and will add a comment inline as well.

Comment 18

[:Paolo Amadini]

Comment on attachment 8680605 [details]
MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

Comment 19

[Tanvi Vyas[:tanvi]]

> (In reply to Tanvi Vyas [:tanvi] from comment #14)
> > ::: browser/base/content/test/general/browser_bug906190.js:178
> > (Diff revision 3)
> > > +  yield doTest(gHttpTestRoot2 + "file_bug906190_1.html",
> > 
> > I think these should both be gHttpTestRoot2 for both of these because you
> > are clicking a child link to the same origin.
> 
> I think you're talking about test 4 (now called
> test_same_origin_metarefresh_different_origin). You're right, the original
> test didn't do what the description in the comment said. I think the same
> goes for test 6 (now called test_same_origin_302redirect_different_origin),
> apparently both links in test 6 should use gHttpTestRoot2 as well.
> 
> I've updated the two test cases and they still pass like they did before.
> I'll include the changes in the next revision of the patch.

Thanks Paolo!  The origins used look good.  Adding some information below to help clarify how the descriptions and tests didn't match up before, and now they do:

In the original code, test 4 takes a gHttpTestRoot1 top level page (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#342) and initiates a link click to a gHttpTestRoot2 page (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#525).  The gHttpTestRoot2 page then does a meta refresh to a gHttpTestRoot1 page (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/file_bug906190_3_4.html?force=1#9).

The description for test 4, on the other hand, says that the top level page should do a link click to same origin page and then that same origin page would meta refresh to a new page.  So both the top level page and the link that is clicked on should use gHttpTestRoot2, as Paolo has done in his updated patch.

In the original code for test 6, we have a gHttpTestRoot1 top level page (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#527).  That clicks a link to an gHttpTestRoot2 page (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#464).  And the gHttpTestRoot2 page redirects to a gHttpTestRoot1 page (https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/file_bug906190.sjs#3).  

The description for test6, on the other hand, says taht the top level page should do a link click to same origin page and then the same origin page would do a 302 redirect to a new page.  So both the top level page and the link that is clicked on should use gHttpTestRoot2, as Paolo has done in his updated patch.

Comment 20

[Tanvi Vyas[:tanvi]]

Comment on attachment 8680605 [details]
MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi

https://reviewboard.mozilla.org/r/23651/#review21757

Comment 21

[Pulsebot]

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

Comment 22

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

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