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.

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

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

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

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.

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 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 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 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

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.

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/

(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?

(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).

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.

(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 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?

(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 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

> (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 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