Right now it fails with: 984 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug906190.js | uncaught exception - NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFocusManager.getFocusedElementForWindow] at chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1554 Stack trace: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1346 null:null:0 985 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug906190.js | Test timed out - expected PASS 986 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug906190.js | Found a browser window after previous test timed out - expected PASS

This test needs waitForFocus to be able to handle child processes in another window from 'window'.

(In reply to Neil Deakin from comment #1) > This test needs waitForFocus to be able to handle child processes in another > window from 'window'. Bug 1212520 removed waitForFocus from the test.

This should make the test more robust and e10s-friendly but still doesn't get it to pass. There are failures such as "Expected state for activeBlocked matches UI state - Got false, expected true" and "identityBox has expected class for activeLoaded - Got false, expected true". Would be nice if someone more familiar with the mixed content UI could look into this. Tanvi?

Comment on attachment 8726170 [details] [diff] [review] WIP >- // Wait for the script in the page to update the contents of the test div. >- let testDiv = content.document.getElementById('mctestdiv'); >- yield promiseWaitForCondition( >- () => testDiv.innerHTML == "Mixed Content Blocker disabled"); >+ yield ContentTask.spawn(browser, childTabSpec, function* (childTabSpec) { >+ // Wait for the script in the page to update the contents of the test div. >+ let testDiv = content.document.getElementById("mctestdiv"); >+ yield new Promise( >+ function (resolve, reject) { >+ let interval = setInterval(function () { >+ if (testDiv.innerHTML == "Mixed Content Blocker disabled") { >+ clearInterval(interval); This should probably use ContentTaskUtils.waitForCondition...

Adding bgrins who is also familiar with browser mixed content tests. I am out next week so won't be able to look at this until the following week.

Now using ContentTaskUtils.waitForCondition. Still the same failures.

Comment on attachment 8727364 [details] [diff] [review] WIP Review of attachment 8727364 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_bug906190.js @@ +64,4 @@ > } > > yield testTaskFn(); > If I stick a `yield new Promise(r=>r)` right here to pause the test and inspect what's going on, I see different behavior in e10s and non-e10s which is causing at least one of the assertion failures (see note below). @@ +114,5 @@ > }); > > + yield ContentTask.spawn(gBrowser.selectedBrowser, null, function* () { > + is(content.document.getElementById('mctestdiv').innerHTML, > + "Mixed Content Blocker disabled", "OK: Executed mixed script"); There's is a difference in behavior in the content's output in e10s, so looks like it's surfacing an actual e10s MCB bug. If I pause execution using the snippet above I notice that the new page says "Mixed Content Blocker disabled" in non-e10s and "Mixed Content Blocker enabled" in e10s. The test is disabling MCB on a page and then opening a link from that page in a new tab. The assertion is checking to see that MCB is also disabled on the new tab, but in e10s MCB is re-enabled. When MCB is disabled BrowserReloadWithFlags(Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT) is called - I'm assuming the platform should be handling persisting the MCB state for tabs opened in that page once that's done. Tanvi, any ideas about this?

(In reply to Brian Grinstead [:bgrins] from comment #7) > The test is disabling MCB on a page and then opening a link from that page > in a new tab. The assertion is checking to see that MCB is also disabled on > the new tab, but in e10s MCB is re-enabled. When MCB is disabled > BrowserReloadWithFlags(Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT) > is called - I'm assuming the platform should be handling persisting the MCB > state for tabs opened in that page once that's done. Tanvi, any ideas about > this? You are right Brian. The problem isn't that the test isn't written properly, the problem is that the desired behavior of MCB + open in new tab is not working in e10s. * Open non-e10s browser. Go to https://people.mozilla.org/~tvyas/mixedcontent.html. Disable Protection. Right click + open link in new tab on link in the page. The new tab has a crossed out lock and loads mixed active content. * Open e10s browser. Go to https://people.mozilla.org/~tvyas/mixedcontent.html. Disable Protection. Right click + open link in new tab on link in the page. The new tab has blocked mixed active content; the decision to disable protection does not persist across to the new tab, as it should per bug https://bugzilla.mozilla.org/show_bug.cgi?id=906190. See http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10790. Maybe mMixedContentChannel is not set properly?

The patches fix the "open in new tab by context menu" case and the "ctrl + click link" case respectively: Part 1 - [Context Menu] |this.browser| in [1] is from [2] in e10s case and it has no docShell (since it's a newly opened remote tab). We can't rely on the child tab browser to decide if its parent allows mixed content. To fix this, we propagate "whether parent allows mixed content" info to before branching to e10s or non-e10s case. In nsContextMenu.openLinkInTab [3], we then do the same origin check and decide if the child tab allows mixed content, too. Part 2 - [CTRL + click] In non-e10s case, the click event wouldn't be handled in [4] because of [5]. Instead, the event would be handled in [6] at first and IPC to [7] where "allowMixedContent" is not taken into account. So the solution is fairly simple: decide if mixed content is allowed in the "local" handler (as compared to "remote" handler) and deliver the allowance flag to [7]. The fixes are not difficult at all but pretty non-scalable due to the current e10s design. This kind of issue doesn't seem to be first and the last one. I don't have a perfect solution to it so just leave the detail that I traced for reference in the future... [1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#986 [2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#4135 [3] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#977 [4] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5277 [5] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#954 [6] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#384 [7] https://dxr.mozilla.org/mozilla-central/source/browser/modules/ContentClick.jsm#32

Hi Tanvi, Per comment 12, could you please help review my patches? Even though I only verified manually (will have an actual try run soon), I am still confident that the root causes are addressed in the patches. Thanks!

Update: both e10s and non-e10s test cases with the patches passed on my desktop. Waiting for try result.

(In reply to Henry Chang [:henry] from comment #14) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=43cb3c0ed11d Hi Henry, Try looks pretty bad but it is not clear whether it is because of these changes or becuase of bug https://bugzilla.mozilla.org/show_bug.cgi?id=1251152. Generally speaking, you should only push one bug to try at a time, unless there are dependencies. Can you repush to try? I will review the patches.

Comment on attachment 8735408 [details] [diff] [review] Part 1: Fix "open by context menu" case This looks okay to me, but I would have a browser peer review as well.

Comment on attachment 8735408 [details] [diff] [review] Part 1: Fix "open by context menu" case Review of attachment 8735408 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me but redirecting to Matt just to be sure

Comment on attachment 8735409 [details] [diff] [review] Part 2: Fix "ctrl + click link" case I don't remember having to specifically call out a difference between ctrl+click and Open Link in New Tab, and implement a solution for both separately. So I'm not sure why this code is needed. >diff --git a/browser/base/content/content.js b/browser/base/content/content.js >@@ -443,16 +443,32 @@ var ClickEventHandler = { >+ json.allowMixedContent = false; >+ let docshell = ownerDoc.defaultView.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation) >+ .QueryInterface(Ci.nsIDocShell); >+ if (docShell.mixedContentChannel) { >+ const sm = Services.scriptSecurityManager; >+ try { >+ var targetURI = BrowserUtils.makeURI(href); >+ sm.checkSameOriginURI(ownerDoc.documentURIObject, targetURI, false); why isn't this: sm.checkSameOriginURI(docshell.mixedContentChannel, targetURI, false); >+ json.allowMixedContent = true; >+ } catch (e) {} >+ } >diff --git a/browser/modules/ContentClick.jsm b/browser/modules/ContentClick.jsm >@@ -76,11 +76,18 @@ var ContentClick = { > let params = { charset: browser.characterSet, > referrerURI: browser.documentURI, > referrerPolicy: json.referrerPolicy, > noReferrer: json.noReferrer }; >+ >+ // In the "open in new tab" case, take "allowMixedContent" into account. >+ // XXX Is 'tab === where' needed? Check with a browser peer. >+ if ("tab" === where && json.allowMixedContent) { >+ params.allowMixedContent = true; >+ } >+ > window.openLinkIn(json.href, where, params); > } > }; Again, I'm not sure why this code is needed now when we didn't have a params for allowMixedContent before. You can also test your implementation with https://people.mozilla.com/~tvyas/mixedcontent.html, which has a link that takes you to another same origin mixed content page. Thanks for taking this on!

Hi Tanvi, Thanks for the review :) I've tested with https://people.mozilla.com/~tvyas/mixedcontent.html, both same origin and cross origin. (the first 2 links are same origin and the third one is cross origin so the third one should always be blocked no matter what.) Without patch part 2, "disable mixed content blocking for now" wouldn't propagate to the new tab after a "ctrl + click". The code to actually "open" a link with mixed content allowed is indeed in one place [1] but I am sure that determining whether we "need" to do that is implemented in separate places [2][3]. (In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #19) > Comment on attachment 8735409 [details] [diff] [review] > Part 2: Fix "ctrl + click link" case > > I don't remember having to specifically call out a difference between > ctrl+click and Open Link in New Tab, and implement a solution for both > separately. So I'm not sure why this code is needed. > > >diff --git a/browser/base/content/content.js b/browser/base/content/content.js > >@@ -443,16 +443,32 @@ var ClickEventHandler = { > >+ json.allowMixedContent = false; > >+ let docshell = ownerDoc.defaultView.QueryInterface(Ci.nsIInterfaceRequestor) > >+ .getInterface(Ci.nsIWebNavigation) > >+ .QueryInterface(Ci.nsIDocShell); > >+ if (docShell.mixedContentChannel) { > >+ const sm = Services.scriptSecurityManager; > >+ try { > >+ var targetURI = BrowserUtils.makeURI(href); > >+ sm.checkSameOriginURI(ownerDoc.documentURIObject, targetURI, false); > why isn't this: > sm.checkSameOriginURI(docshell.mixedContentChannel, targetURI, false); > > >+ json.allowMixedContent = true; > >+ } catch (e) {} > >+ } I didn't know we can do it like this :) I'll have a try. Thanks! > > >+ if ("tab" === where && json.allowMixedContent) { > >+ params.allowMixedContent = true; > >+ } > >+ > > window.openLinkIn(json.href, where, params); > > } > > }; > Again, I'm not sure why this code is needed now when we didn't have a params > for allowMixedContent before. > It's because the non-e10s mode goes to other click event handler in browser.js::handleClickEvent, where "allowMixedContent" would be added to |param| whenever needed. > You can also test your implementation with > https://people.mozilla.com/~tvyas/mixedcontent.html, which has a link that > takes you to another same origin mixed content page. > > Thanks for taking this on! [1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1810 [2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#986 [3] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5382 [4] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5406

(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #16) > (In reply to Henry Chang [:henry] from comment #14) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=43cb3c0ed11d > > Hi Henry, > > Try looks pretty bad but it is not clear whether it is because of these > changes or becuase of bug > https://bugzilla.mozilla.org/show_bug.cgi?id=1251152. Generally speaking, > you should only push one bug to try at a time, unless there are > dependencies. Can you repush to try? > > I will review the patches. Hi Tanvi, Sorry it's my fault :( The try result was messed up with my patches for other bug. I'll push to try again! Thanks!

Comment on attachment 8735408 [details] [diff] [review] Part 1: Fix "open by context menu" case Review of attachment 8735408 [details] [diff] [review]: ----------------------------------------------------------------- This is fine without the "parent" in the variable/property naming that isn't actually related to "parents". Re-flag me for review if you want more feedback or disagree. ::: browser/base/content/content.js @@ +110,5 @@ > .outerWindowID; > let loginFillInfo = LoginManagerContent.getFieldContext(event.target); > > + // The same-origin check will be done in nsContextMenu.openLinkInTab. > + let parentAllowsMixedContent = !!docShell.mixedContentChannel; I think this is a bit confusing because the name kinda implies we're getting the answer from the parent but we're really trusting the content process to relay the proper value from the docshell. It seems like in this case "parent" is referring to the page the click is in (like the opener) and isn't related to the process. I don't think this property name should mention "parent" at all in that case since it doesn't make sense in general. How about calling it `mixedContentAllowed`?

Hi Matthew, Thanks for the review :) The reason I added a prefix to "allowMixedContent" is the flag only indicates if the opener allows mixed content. The opened page has to do the same origin check even if the opener is allowed to load mixed content. What do you think of "openerMixedContentAllowed"? Thanks :) (In reply to Matthew N. [:MattN] from comment #23) > Comment on attachment 8735408 [details] [diff] [review] > Part 1: Fix "open by context menu" case > > Review of attachment 8735408 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is fine without the "parent" in the variable/property naming that isn't > actually related to "parents". > > Re-flag me for review if you want more feedback or disagree. > > ::: browser/base/content/content.js > @@ +110,5 @@ > > .outerWindowID; > > let loginFillInfo = LoginManagerContent.getFieldContext(event.target); > > > > + // The same-origin check will be done in nsContextMenu.openLinkInTab. > > + let parentAllowsMixedContent = !!docShell.mixedContentChannel; > > I think this is a bit confusing because the name kinda implies we're getting > the answer from the parent but we're really trusting the content process to > relay the proper value from the docshell. > > It seems like in this case "parent" is referring to the page the click is in > (like the opener) and isn't related to the process. I don't think this > property name should mention "parent" at all in that case since it doesn't > make sense in general. > > How about calling it `mixedContentAllowed`?

(In reply to Henry Chang [:henry] from comment #24) > > What do you think of "openerMixedContentAllowed"? Thanks :) > Or referrerMixedContentAllowed? Let's see what Matt says.

Hi Henry, Thanks for your explanation. I understand why we need the additional code now. Can you please update the patch per and reflag me for review?

Comment on attachment 8739292 [details] [diff] [review] Part 2: Fix "ctrl + click link" case Hi Tanvi, I replaced ownerDoc.documentURIObject with docshell.mixedContentChannel.URI in the this updated patch. Could you have it a review again? Thanks :)

(In reply to Henry Chang [:henry] from comment #24) > Hi Matthew, > > Thanks for the review :) > > The reason I added a prefix to "allowMixedContent" is the flag only > indicates if the opener allows mixed content. The opened page has to do the > same origin check even if the opener is allowed to load mixed content. Unless I'm missing something, all of the code in this patch is running in the opener not the opened page so that's why I think the prefix is confusing. I would be fine with the prefix in the context of the new page but that's not the case for this patch AFAICS.

(In reply to Matthew N. [:MattN] from comment #30) > (In reply to Henry Chang [:henry] from comment #24) > > Hi Matthew, > > > > Thanks for the review :) > > > > The reason I added a prefix to "allowMixedContent" is the flag only > > indicates if the opener allows mixed content. The opened page has to do the > > same origin check even if the opener is allowed to load mixed content. > > Unless I'm missing something, all of the code in this patch is running in > the opener not the opened page so that's why I think the prefix is > confusing. I would be fine with the prefix in the context of the new page > but that's not the case for this patch AFAICS. Hi Matthew, I got your point and it makes sense to me! I'll use the name without prefix. Thanks :)

Comment on attachment 8739292 [details] [diff] [review] Part 2: Fix "ctrl + click link" case r+ from me, but please get a browser peer review for the below. Please repost to try and/or run "./mach mochitest mcb" locally. (In reply to Tanvi Vyas [:tanvi] from comment #19) > Comment on attachment 8735409 [details] [diff] [review] > Part 2: Fix "ctrl + click link" case > >diff --git a/browser/modules/ContentClick.jsm b/browser/modules/ContentClick.jsm > >@@ -76,11 +76,18 @@ var ContentClick = { > > let params = { charset: browser.characterSet, > > referrerURI: browser.documentURI, > > referrerPolicy: json.referrerPolicy, > > noReferrer: json.noReferrer }; > >+ > >+ // In the "open in new tab" case, take "allowMixedContent" into account. > >+ // XXX Is 'tab === where' needed? > Check with a browser peer. >

(In reply to Tanvi Vyas [:tanvi] from comment #32) > Comment on attachment 8739292 [details] [diff] [review] > Part 2: Fix "ctrl + click link" case > > r+ from me, but please get a browser peer review for the below. > > Please repost to try and/or run "./mach mochitest mcb" locally. > > (In reply to Tanvi Vyas [:tanvi] from comment #19) > > Comment on attachment 8735409 [details] [diff] [review] > > Part 2: Fix "ctrl + click link" case > > >diff --git a/browser/modules/ContentClick.jsm b/browser/modules/ContentClick.jsm > > >@@ -76,11 +76,18 @@ var ContentClick = { > > > let params = { charset: browser.characterSet, > > > referrerURI: browser.documentURI, > > > referrerPolicy: json.referrerPolicy, > > > noReferrer: json.noReferrer }; > > >+ > > >+ // In the "open in new tab" case, take "allowMixedContent" into account. > > >+ // XXX Is 'tab === where' needed? > > Check with a browser peer. > > Thanks Tanvi :) Will ask check with a browser peer pretty soon.

Hi Jared, Could you suggest us if "tab === where" is needed in patch part 2? Briefly speaking, flag |allowMixedContent| is forgotten to send to |window.openLinkIn| in [1] to let the opened tab knows if its opener temporarily disable mixed content blocking. I added that check just because the flag is only useful in the "open in the new tab" case. Do you think if it's okay with you, too? Thanks :) [1] https://dxr.mozilla.org/mozilla-central/source/browser/modules/ContentClick.jsm#84

Hi Matt, Could you also give some feedback per comment 34? I tend to have tab === "where" in the patch. Do you have any concern regarding this? Thanks :)

Comment on attachment 8739292 [details] [diff] [review] Part 2: Fix "ctrl + click link" case Review of attachment 8739292 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/ContentClick.jsm @@ +83,5 @@ > noReferrer: json.noReferrer }; > + > + // In the "open in new tab" case, take "allowMixedContent" into account. > + // XXX Is 'tab === where' needed? > + if ("tab" === where && json.allowMixedContent) { Other values I see in whereToOpenLink: "tabshifted" - I don't see why we would exclude it "save" - Doesn't seem like it will be an issue as the page contents shouldn't be running "window" - I'm guessing this is handled some other way already? "current" - I'm guessing this is handled by some existing saved state for the frame/docshell. It seems to me like it should be fine to send this property unconditionally as long as downstream functions don't throw due to unknown properties: ```allowMixedContent: json.allowMixedContent```

Comment on attachment 8739292 [details] [diff] [review] Part 2: Fix "ctrl + click link" case Review of attachment 8739292 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content.js @@ +451,5 @@ > } > } > json.noReferrer = BrowserUtils.linkHasNoReferrer(node) > > + // Check if the link needs to be opended with mixed content allowed. Nit: typo: "opended" @@ +461,5 @@ > + .QueryInterface(Ci.nsIDocShell); > + if (docShell.mixedContentChannel) { > + const sm = Services.scriptSecurityManager; > + try { > + var targetURI = BrowserUtils.makeURI(href); Nit: use `let` instead of `var`

Thanks Matt! Running on try and ready to push if the result is good. Thanks! (In reply to Matthew N. [:MattN] (behind on reviews) from comment #38) > Comment on attachment 8739292 [details] [diff] [review] > Part 2: Fix "ctrl + click link" case > > Review of attachment 8739292 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/content.js > @@ +451,5 @@ > > } > > } > > json.noReferrer = BrowserUtils.linkHasNoReferrer(node) > > > > + // Check if the link needs to be opended with mixed content allowed. > > Nit: typo: "opended" > > @@ +461,5 @@ > > + .QueryInterface(Ci.nsIDocShell); > > + if (docShell.mixedContentChannel) { > > + const sm = Services.scriptSecurityManager; > > + try { > > + var targetURI = BrowserUtils.makeURI(href); > > Nit: use `let` instead of `var`

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f50abd87ae21 try looks good :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/220a41ef6ffa https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b548af3126

https://hg.mozilla.org/mozilla-central/rev/220a41ef6ffa https://hg.mozilla.org/mozilla-central/rev/c6b548af3126

Did this miss removing the skip-if e10s for this test from the manifest file?

I turned on the e10s mode and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfd47a3254c1&selectedJob=20031339 browser_bug906190.js got PASSed but there seems memory leak issue in the test chunk: http://archive.mozilla.org/pub/firefox/try-builds/hchang@mozilla.com-cfd47a3254c1bd9b2a62a8098ae2200ee4e0200d/try-linux-debug/try_ubuntu32_vm-debug_test-mochitest-e10s-browser-chrome-7-bm07-tests1-linux32-build1756.txt.gz Not sure if it's a common issue so pushed a clean code base to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b1b9eccf92e

I can reproduce the memory leak issue on debug build on my desktop (after turning on e10s mochitest). See attachment 8745991 [details] for the detail. Currently have no idea what's happening ... Tanvi, Christoph, since you ever changed browser_bug906190.js, what do you think of the memory leak issue? I will try to investigate tomorrow but just like to know if you have any idea about this. Thanks :)

At a hunch, does replacing: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#60 with yield BrowserTestUtils.removeTab(gBrowser.selectedTab); help?

(In reply to :Gijs Kruitbosch from comment #49) > with yield BrowserTestUtils.removeTab(gBrowser.selectedTab); I think Gijs might already provide the right hint. Most likely we are forgetting to remove a tab. Let's see if his suggestion already fixes the leak.

Unfortunately it doesn't fix the leak :( I tried removing "yield BrowserTestUtils.removeTab" in some other test cases and it doesn't result in memory leak. Maybe it's not the root cause of the leak. BTW, the leak only happens in e10s mode.

It's sub test case 5 and 6 [1] that causes the memory leak so I suspect it's due to "redirect". Digging right now! [1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#191

Should this bug be reopened?

(In reply to Tanvi Vyas [:tanvi] from comment #53) > Should this bug be reopened? No, not unless we back out the code that landed. This bug summary should be renamed since neither of the two fixes are specifically about the test and a new bug should be filed specifically for getting the test working.