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

RESOLVED FIXED in Firefox 45

Status

()

Firefox
Security
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 45
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

3 years ago
There's also bug 1093642 to make this compatible with e10s. That would also be easier if the test is converted to use Tasks.
Blocks: 1093642
I don't feel very comfortable about disabling browser_bug906190.js on Linux.  We should fix this test ASAP and re-enable it.
Whiteboard: [fxprivacy]

Updated

3 years ago
Whiteboard: [fxprivacy] → [fxprivacy] [triage]

Updated

3 years ago
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]

Updated

3 years ago
Flags: qe-verify?
Priority: P2 → P1
(Assignee)

Updated

3 years ago
Flags: qe-verify? → qe-verify-
See Also: → bug 1093642
(Assignee)

Updated

3 years ago
Assignee: nobody → paolo.mozmail
Iteration: --- → 44.3 - Nov 2
(Assignee)

Comment 3

3 years ago
Created 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
(Assignee)

Comment 4

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

Updated

3 years ago
Status: NEW → ASSIGNED

Updated

3 years ago
Blocks: 1188121
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

3 years ago
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.
Attachment #8680605 - Flags: feedback?(tanvi)
(Assignee)

Updated

3 years ago
Summary: Rewrite Mixed Content Blocking browser-chrome tests with Tasks and re-enable browser_bug906190.js on Linux → Rewrite browser_bug906190.js with Tasks and re-enable it on Linux
(Assignee)

Comment 8

3 years ago
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
Attachment #8680605 - Attachment description: MozReview Request: Bug 1212520 - Rewrite Mixed Content Blocking browser-chrome tests with Tasks and re-enable browser_bug906190.js on Linux. r=bgrins → MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=bgrins
Attachment #8680605 - Flags: feedback?(tanvi) → review?(bgrinstead)
(Assignee)

Updated

3 years ago
Attachment #8680605 - Flags: feedback?(tanvi)
(Assignee)

Comment 9

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

Comment 10

3 years ago
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?
(Assignee)

Comment 12

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

Comment 15

3 years ago
(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?
Attachment #8680605 - Flags: review?(bgrinstead)

Updated

3 years ago
Iteration: 44.3 - Nov 2 → 45.1 - Nov 16
(Assignee)

Updated

3 years ago
Blocks: 1221114
(Assignee)

Comment 17

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

Comment 18

3 years ago
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
Attachment #8680605 - Attachment description: MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=bgrins → MozReview Request: Bug 1212520 - Rewrite browser_bug906190.js with Tasks and re-enable it on Linux. r=tanvi
Attachment #8680605 - Flags: feedback?(tanvi) → review?(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.
Attachment #8680605 - Flags: review?(tanvi) → review+
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 22

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3fc6afb1021
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Updated

2 years ago
See Also: bug 1093642
You need to log in before you can comment on or make changes to this bug.