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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

Bug 1237799 seems like it could fix the rest of test_main.html with e10s.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Tanvi Vyas[:tanvi]]

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!

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Set prefs with pushPrefEnv in mixedcontentblocker/test_main.html.

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

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 12

[Tanvi Vyas[:tanvi]]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb573ec97cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/cac3074b9250
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2efffe5e6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/27a46c84100e

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/12019861dc72
https://hg.mozilla.org/releases/mozilla-aurora/rev/3193a521d6c2
https://hg.mozilla.org/releases/mozilla-aurora/rev/abc810732d73
https://hg.mozilla.org/releases/mozilla-aurora/rev/860b4f829e02