Fix mixedcontentblocker mochitests to set prefs in an e10s-compatible way

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 fixed, firefox47 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
dom/security/test/mixedcontentblocker/test_bug803225.html and dom/security/test/mixedcontentblocker/test_main.html are disabled with e10s. The first problem with these tests is that they set preferences in a way that does not work with e10s. I have some patches for that.
(Assignee)

Comment 1

2 years ago
With these patches test_bug803225.html seems to mostly work, except for the mailto and wss protocols. I suspect those may not be set up correctly to work with e10s. test_main.html fails immediately because it tries to use SpecialPowers.Ci.nsIPluginHost, which does not work from the child process.
(Assignee)

Comment 2

2 years ago
Bug 1237799 seems like it could fix the rest of test_main.html with e10s.
(Assignee)

Comment 3

2 years ago
Created attachment 8705390 [details] [diff] [review]
Make setting of preferences in mixedcontentblocker tests e10s-compatible.

This just callback-izes things. Most of the top level of these tests
are function declarations, so I could move things around without
interfering with how things run, as far as I can tell.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a111fbd55a6
Attachment #8705390 - Flags: review?(tanvi)

Updated

2 years ago
Blocks: 984139
tracking-e10s: --- → +

Comment 4

2 years ago
Comment on attachment 8705390 [details] [diff] [review]
Make setting of preferences in mixedcontentblocker tests e10s-compatible.

diff --git a/dom/security/test/mixedcontentblocker/test_main.html b/dom/security/test/mixedcontentblocker/test_main.html
index 1ccbfe5..73873a8 100644
--- a/dom/security/test/mixedcontentblocker/test_main.html
+++ b/dom/security/test/mixedcontentblocker/test_main.html
>-  // listen for a messages from the mixed content test harness
>-  window.addEventListener("message", receiveMessage, false);
>+  //Set the first set of settings (true, true) and increment the counter.
>+  changePrefs(function() {
>+    // listen for a messages from the mixed content test harness
>+    window.addEventListener("message", receiveMessage, false);
>+
>+    // Enable <picture> and <img srcset> for test
>+    SpecialPowers.pushPrefEnv({'set': [[ "dom.image.srcset.enabled", true ],
>+                                       [ "dom.image.picture.enabled", true ]] },
>+                              function() {
>+      // Kick off test
>+      reloadFrame();
>+    });
nit: odd indentation of callback.  Either indent it all under pushPrefEnv(), or move "function() {" back inline with the rest of the function.

Alternatively, what if we move the image settings into changePrefs() and do a pushPrefEnv for them there when counter=0?  You might end up with nested callbacks then.  It would be nice if we could do this in one pushPrefEnv call.
>+  });
>+



diff --git a/dom/security/test/mixedcontentblocker/test_bug803225.html b/dom/security/test/mixedcontentblocker/test_bug803225.html
index 9b6f6af..494535c 100644
--- a/dom/security/test/mixedcontentblocker/test_bug803225.html
+++ b/dom/security/test/mixedcontentblocker/test_bug803225.html
>-  // listen for a messages from the mixed content test harness
>-  window.addEventListener("message", receiveMessage, false);
>+  //Set the first set of settings (true, true) and increment the counter.
>+  changePrefs(function () {
>+    // listen for a messages from the mixed content test harness
>+    window.addEventListener("message", receiveMessage, false);
>+  });

In this test, the frame is already loaded (you don't have to call reloadFrame() to kick off the test).  Hence, you may get messages before you have changed the preferences.  Those messages would be correctly ignored since at that point there would be no event listener.  But instead, we should remove the contents of the iframe and just have <iframe id="testHarness"></iframe>.  Then add a loadFrame() or reloadFrame() method that sets its source, like we have in test_main.html.  So that we don't have an unnecessary iframe load with postMessages that are being thrown away.  Make sense?

Overall, this looks good to me, but I'd like to see the updates so r- for now.  Thanks for picking up this work!
Attachment #8705390 - Flags: review?(tanvi) → review-

Updated

2 years ago
Duplicate of this bug: 1217579
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
Created attachment 8711122 [details] [diff] [review]
part 1 - Set prefs with pushPrefEnv in mixedcontentblocker/test_main.html.

test_main is done, so I'll just split this into a separate patch.

Rather than key off of the value of the counter, I added an extra argument to pass in additional prefs to set. Probably more general than is needed.
Attachment #8711122 - Flags: review?(tanvi)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8711122 [details] [diff] [review]
part 1 - Set prefs with pushPrefEnv in mixedcontentblocker/test_main.html.

I'm going to change something else in here, so I'm canceling the review for now.
Attachment #8711122 - Attachment is obsolete: true
Attachment #8711122 - Flags: review?(tanvi)
(Assignee)

Updated

2 years ago
Blocks: 1190763
(Assignee)

Comment 8

2 years ago
Created attachment 8711246 [details] [diff] [review]
part 1 - Set prefs with pushPrefEnv in mixedcontentblocker/test_main.html.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=751a3688b4c4
Attachment #8711246 - Flags: review?(tanvi)
(Assignee)

Updated

2 years ago
Attachment #8705390 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8711247 [details] [diff] [review]
part 2 - Don't start running mixedcontentblocker/test_main.html until the page has finished loading.

If the script runs before the page finishes loading, you get an error
about getElementById being null.  This doesn't seem to usually happen
with this particular test, but it looks like it can happen at least
sometimes.
Attachment #8711247 - Flags: review?(tanvi)
(Assignee)

Comment 10

2 years ago
Created attachment 8711248 [details] [diff] [review]
part 3 - Don't load an iframe until we're actually ready to run the test in test_bug803225.html.

This addresses your review comment about using a reloadFrame() in the
test.

This needs to wait for onload to do the reloadFrame(), or the
.getElementById() will return null.
Attachment #8711248 - Flags: review?(tanvi)
(Assignee)

Comment 11

2 years ago
Created attachment 8711249 [details] [diff] [review]
part 4 - Convert test_bug803225.html to use pushPrefEnv() to set preferences.

With this patch, the remaining issue to getting test_bug803225.html
working with e10s is the mailto portion of the test, which uses
nsIWebHandlerApp to shim up some kind of fake mailto handler. If I
can't figure out quickly how to fix it, I'll file a separate bug for
the remaining work and land what I have.
Attachment #8711249 - Flags: review?(tanvi)
(Assignee)

Updated

2 years ago
Blocks: 1242775

Comment 12

2 years ago
Comment on attachment 8711246 [details] [diff] [review]
part 1 - Set prefs with pushPrefEnv in mixedcontentblocker/test_main.html.

+  //Set the first set of settings (true, true) and increment the counter.
Change comment to:
//Set the first set of mixed content settings and increment the counter.

+  // Enable <picture> and <img srcset> for the test.
Use "//Enable", just so that you are consistent with the rest of the test.
+  changePrefs([[ "dom.image.srcset.enabled", true ], [ "dom.image.picture.enabled", true ]],
+    function() {
+      // listen for a messages from the
same here.  //listen
Attachment #8711246 - Flags: review?(tanvi) → review+

Updated

2 years ago
Attachment #8711247 - Flags: review?(tanvi) → review+

Updated

2 years ago
Attachment #8711248 - Flags: review?(tanvi) → review+

Updated

2 years ago
Attachment #8711249 - Flags: review?(tanvi) → review+

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ceb573ec97cc
https://hg.mozilla.org/mozilla-central/rev/cac3074b9250
https://hg.mozilla.org/mozilla-central/rev/ea2efffe5e6f
https://hg.mozilla.org/mozilla-central/rev/27a46c84100e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.