e10s - Fix browser_bug906190.js to work in e10s mode

RESOLVED FIXED in Firefox 48

Status

()

Core
Document Navigation
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Gijs, Assigned: henry)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox48 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

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
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
tracking-e10s: --- → +

Comment 1

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

Updated

2 years ago
Depends on: 1212520
See Also: → bug 1212520
(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.
See Also: bug 1212520
Created attachment 8726170 [details] [diff] [review]
WIP

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?
Flags: needinfo?(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.
Created attachment 8727364 [details] [diff] [review]
WIP

Now using ContentTaskUtils.waitForCondition. Still the same failures.
Attachment #8726170 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
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?
Flags: needinfo?(bgrinstead)
(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?
Flags: needinfo?(tanvi)
(Assignee)

Comment 9

a year ago
Created attachment 8735397 [details] [diff] [review]
0001-Bug-1093642-Use-parentAllowsMixedContent-to-decide-i.patch
(Assignee)

Comment 10

a year ago
Created attachment 8735408 [details] [diff] [review]
Part 1: Fix "open by context menu" case
Attachment #8735397 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
Created attachment 8735409 [details] [diff] [review]
Part 2: Fix "ctrl + click link" case
(Assignee)

Comment 12

a year ago
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
(Assignee)

Comment 13

a year ago
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!
Flags: needinfo?(tanvi)
(Assignee)

Comment 14

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43cb3c0ed11d
(Assignee)

Updated

a year ago
Attachment #8735408 - Flags: review?(tanvi)
(Assignee)

Updated

a year ago
Attachment #8735409 - Flags: review?(tanvi)
(Assignee)

Updated

a year ago
Assignee: nobody → hchang
(Assignee)

Comment 15

a year ago
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.
Flags: needinfo?(tanvi)
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.
Attachment #8735408 - Flags: review?(tanvi) → review+
Attachment #8735408 - Flags: review?(bgrinstead)
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
Attachment #8735408 - Flags: review?(bgrinstead) → review?(MattN+bmo)
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!
Attachment #8735409 - Flags: review?(tanvi) → review-
(Assignee)

Comment 20

a year ago
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
Flags: needinfo?(tanvi)
(Assignee)

Comment 21

a year ago
(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!
(Assignee)

Comment 22

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad2f1969eb51
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`?
Attachment #8735408 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 24

a year ago
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`?
Flags: needinfo?(MattN+bmo)
(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?
Flags: needinfo?(tanvi)
(Assignee)

Comment 27

a year ago
Created attachment 8739291 [details] [diff] [review]
Part 2: Fix "ctrl + click link" case
(Assignee)

Comment 28

a year ago
Created attachment 8739292 [details] [diff] [review]
Part 2: Fix "ctrl + click link" case
Attachment #8735409 - Attachment is obsolete: true
Attachment #8739291 - Attachment is obsolete: true
(Assignee)

Comment 29

a year ago
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 :)
Attachment #8739292 - Flags: review?(tanvi)
(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.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 31

a year ago
(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.
>
Attachment #8739292 - Flags: review?(tanvi) → review+
(Assignee)

Comment 33

a year ago
(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.
(Assignee)

Comment 34

a year ago
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
Flags: needinfo?(jaws)
(Assignee)

Comment 35

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8416af78a4e1
(Assignee)

Comment 36

a year ago
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 :)
Flags: needinfo?(MattN+bmo)
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```
Flags: needinfo?(MattN+bmo)
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`
(Assignee)

Comment 39

a year ago
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`
Flags: needinfo?(jaws)
(Assignee)

Comment 40

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f50abd87ae21

try looks good :)
(Assignee)

Comment 41

a year ago
Created attachment 8743778 [details] [diff] [review]
Part1.patch (r+'ed)
Attachment #8727364 - Attachment is obsolete: true
Attachment #8735408 - Attachment is obsolete: true
Attachment #8739292 - Attachment is obsolete: true
(Assignee)

Comment 42

a year ago
Created attachment 8743779 [details] [diff] [review]
Part2.patch (r+'ed)
(Assignee)

Updated

a year ago
Attachment #8743778 - Attachment filename: Part1.patch → Part1.patch (r+'ed)
(Assignee)

Updated

a year ago
Attachment #8743778 - Attachment description: Part1.patch → Part1.patch (r+'ed)
Attachment #8743778 - Attachment filename: Part1.patch (r+'ed) → Part1.patch
(Assignee)

Updated

a year ago
Attachment #8743779 - Attachment description: Part2.patch → Part2.patch (r+'ed)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 43

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/220a41ef6ffa
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b548af3126
Keywords: checkin-needed

Comment 44

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/220a41ef6ffa
https://hg.mozilla.org/mozilla-central/rev/c6b548af3126
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Did this miss removing the skip-if e10s for this test from the manifest file?
Flags: needinfo?(hchang)
(Assignee)

Comment 46

a year ago
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
Flags: needinfo?(hchang)
(Assignee)

Comment 47

a year ago
Created attachment 8745991 [details]
memory-leak.log
(Assignee)

Comment 48

a year ago
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 :)
Flags: needinfo?(tanvi)
Flags: needinfo?(ckerschb)
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.
Flags: needinfo?(ckerschb)
Flags: needinfo?(tanvi)
(Assignee)

Comment 51

a year ago
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.
(Assignee)

Comment 52

a year ago
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.
You need to log in before you can comment on or make changes to this bug.