1133577 - [e10s] "Open Link in New Tab" in remote browser causes unsafe CPOW usage warning

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) Visit a site with some links on it in an e10s window
2) Right-click on a link, and choose "Open Link in New Tab"

This causes some "unsafe CPOW usage" warnings in the Browser Console.

In browser/base/content/nsContextMenu.js:

  // Open linked-to URL in a new tab.
  openLinkInTab: function() {
    var doc = this.target.ownerDocument; <-- Causes CPOW warning
    urlSecurityCheck(this.linkURL, this.principal);
    var referrerURI = doc.documentURIObject; <-- Causes CPOW warning

    // if the mixedContentChannel is present and the referring URI passes
    // a same origin check with the target URI, we can preserve the users
    // decision of disabling MCB on a page for it's child tabs.
    var persistAllowMixedContentInChildTab = false;

    if (this.browser.docShell && this.browser.docShell.mixedContentChannel) {
      const sm = Services.scriptSecurityManager;
      try {
        var targetURI = this.linkURI;
        sm.checkSameOriginURI(referrerURI, targetURI, false);
        persistAllowMixedContentInChildTab = true;
      }
      catch (e) { }
    }

    let params = this._openLinkInParameters(doc, {
      allowMixedContent: persistAllowMixedContentInChildTab,
    });
    openLinkIn(this.linkURL, "tab", params);
  },

...

  _openLinkInParameters : function (doc, extra) {
    let params = { charset: doc.characterSet,
                   referrerURI: doc.documentURIObject, <-- Causes CPOW warning
                   noReferrer: BrowserUtils.linkHasNoReferrer(this.link) };
    for (let p in extra)
      params[p] = extra[p];
    return params;
  },

in toolkit/modules/BrowserUtils.jsm:

  /**
   * Return true if linkNode has a rel="noreferrer" attribute.
   *
   * @param linkNode The <a> element, or null.
   * @return a boolean indicating if linkNode has a rel="noreferrer" attribute.
   */
  linkHasNoReferrer: function (linkNode) {
    if (!linkNode)
      return false;

    let rel = linkNode.getAttribute("rel"); <-- Causes CPOW warning
    if (!rel)
      return false;

    // The HTML spec says that rel should be split on spaces before looking
    // for particular rel values.
    let values = rel.split(/[ \t\r\n\f]/);
    return values.indexOf('noreferrer') != -1;
  },

in toolkit/modules/RemoteWebNavigation.jsm

  loadURI: function(aURI, aLoadFlags, aReferrer, aPostData, aHeaders) {
    if (aPostData || aHeaders)
      throw Components.Exception("RemoteWebNavigation doesn't accept postdata or headers.", Cr.NS_ERROR_INVALID_ARGS);

    this._browser._contentTitle = "";
    this._sendMessage("WebNavigation:LoadURI", {
      uri: aURI,
      flags: aLoadFlags,
      referrer: aReferrer ? aReferrer.spec : null <-- Causes CPOW warning
    });
  },

Comment 1

[Ian Moody [:Kwan] (UTC+0)]

Attachment: WIP patch

Work in progress patch I'm putting here for safe keeping.
Fixes all but one CPOW, the linkHasNoReferrer one, which I need to think about more.
Also fixes bug 1134227 for "[...] New Window", and the as-yet unfiled bug for "[...] Private Window"
Depends on the work done in bug 1134708 for the equivalent frame commands.

Comment 2

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8567156 [details] [diff] [review]
WIP patch

(In reply to Ian Moody [:Kwan] from comment #1)
> Created attachment 8567156 [details] [diff] [review]
> WIP patch
> 
> Work in progress patch I'm putting here for safe keeping.
> Fixes all but one CPOW, the linkHasNoReferrer one, which I need to think
> about more.
> Also fixes bug 1134227 for "[...] New Window", and the as-yet unfiled bug
> for "[...] Private Window"
> Depends on the work done in bug 1134708 for the equivalent frame commands.

This is currently all wrong because I'm constructing the URI using the document.referrer URL rather than the document.location.href as I should be.  Getting mixed up over naming, especially on top of the frame patch.  Need to go back through both.

Comment 3

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Make context-menu open link/frame commands use messages to avoid unsafe CPOW warnings

Patch for all the open link and open frame commands, since the changes for one set build on the other.
This will clash with bug 1075670 (patch 4 specifically)

Testing completed locally for all commands in e10s and non-e10s

Comment 4

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Ian Moody [:Kwan] from comment #3)
> Created attachment 8567387 [details] [diff] [review]
> Make context-menu open link/frame commands use messages to avoid unsafe CPOW
> warnings
> 
> Patch for all the open link and open frame commands, since the changes for
> one set build on the other.
> This will clash with bug 1075670 (patch 4 specifically)
> 
> Testing completed locally for all commands in e10s and non-e10s

Hmm, I just remembered that my original patch for bug 1134708 left frameURL as an empty string when we weren't actually in a frame.  Something that got lost in the reshuffling and refactoring. There's no behavioural change since it's only accessed by the three open frame functions, but something to consider for neatness.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8567387 [details] [diff] [review]
Make context-menu open link/frame commands use messages to avoid unsafe CPOW warnings

Review of attachment 8567387 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

Waiting for results from a try run:

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

Comment 6

[Ian Moody [:Kwan] (UTC+0)]

Failure in bc1 in
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug734076.js
reason:
"JavaScript error: chrome://browser/content/nsContextMenu.js, line 942: TypeError: gContextMenuContentData is null"
hmm, I must confess I'm not sure how to fix this.  Is the problem in the test, in how it's creating the context menu? Or should gContextMenuContentData be getting set regardless and the problem's with that?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Let me see if I can reproduce this locally, one sec...

Comment 8

[Mike Conley (:mconley) (:⚙️)]

The problem is here: 

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug734076.js#69

This test is calling directly into nsContextMenu's showOnlyThisFrame, but doing that doesn't populate gContextMenuContentData. I think this test is going to need to be updated in order to trigger the context menu event that populates gContextMenuContentData.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Let me know if you need any help with that.

Comment 10

[Ian Moody [:Kwan] (UTC+0)]

Attachment: WIP "fix" test browser_bug734076.js to use synthesized mouse events

The fix that doesn't fix.

The view background and image tests don't currently have command-clicking code because the context menus don't come up anyway, and the show only clicking code is commented out because it doesn't work.

Comment 11

[Ian Moody [:Kwan] (UTC+0)]

Attachment: fix test browser_bug734076.js to use mouse events to open the context menu so the menu inits correctly

Let's see how this goes with a limited try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e4c93a070ae

Comment 12

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Make context-menu open link/frame commands use messages to avoid unsafe CPOW warnings

Waited to post this until bug 1075670 landed so I could rebase on top of it

Changes since you looked at it before in comment #5:
– changed .documentURI to .documentURIObject to prevent confusion, since documentURI on an actual doc is different
– got rid of .frameURL since it's redundant.  Just use .docLocation directly (should it be .docLocationHref?)
– noticed I missed passing along linkHasNoReferrer in tabbrowser.xml for the e10s case
– variable ordering is slightly different for neatness

Comment 13

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Fix test browser_bug734076.js to open the context menu with mouse events so the menu inits correctly

Changes since previous:
– realised my DRY function was always called after sendMouseEvent(), so moved that into it as well

New try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0caf06264178

Comment 14

[Ian Moody [:Kwan] (UTC+0)]

Hmm, after working on some other code, I can't help but think I should have put linkHasNoReferrer in nsContextMenu.setTarget(), with all the other link stuff, rather than content.js.  In fact, I don't even think it'll always provide the correct results currently.  Clicking on an element child of a <a/>, for instance.

Comment 15

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Make context-menu open link/frame commands use messages to avoid unsafe CPOW warnings

(In reply to Ian Moody [:Kwan] from comment #14)
> Hmm, after working on some other code, I can't help but think I should have
> put linkHasNoReferrer in nsContextMenu.setTarget(), with all the other link
> stuff, rather than content.js.  In fact, I don't even think it'll always
> provide the correct results currently.  Clicking on an element child of a
> <a/>, for instance.
Ah yes, I was right.  Child elements (eg an image) do leave the referrer on rel="noreferrer" links with my previous patch.  Here's one that fixes that, tested manually.

Also, it appears there are currently no tests for this functionality.  There does appear to be one test just for clicking on a link, but none for context-menu actions.
https://dxr.mozilla.org/mozilla-central/source/docshell/test/test_bug530396.html

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8571664 [details] [diff] [review]
Fix test browser_bug734076.js to open the context menu with mouse events so the menu inits correctly

Review of attachment 8571664 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good!

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8571953 [details] [diff] [review]
Make context-menu open link/frame commands use messages to avoid unsafe CPOW warnings

Review of attachment 8571953 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a satisfactory answer to my question below.

::: browser/base/content/nsContextMenu.js
@@ +738,5 @@
>            this.linkURI = this.getLinkURI();
>            this.linkProtocol = this.getLinkProtocol();
>            this.onMailtoLink = (this.linkProtocol == "mailto");
>            this.onSaveableLink = this.isLinkSaveable( this.link );
> +          this.linkHasNoReferrer = BrowserUtils.linkHasNoReferrer(elem);

Is this going to cause an unsafe CPOW usage when we attempt to determine if we have rel="noreferrer" on this node? If not, why not?

If so, can we compute this in the content process like you're doing for the other values, like the documentURI, charset, etc?

Comment 18

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> > +          this.linkHasNoReferrer = BrowserUtils.linkHasNoReferrer(elem);
> 
> Is this going to cause an unsafe CPOW usage when we attempt to determine if
> we have rel="noreferrer" on this node? If not, why not?

It is not, and to be honest, I'm not 100% sure why not.  I would have expected all the stuff in setTarget() to be spitting out CPOWs left and right, but they don't.  I know the elem is (originally) gContextMenuContentData.event.target, but I would not have thought that'd be enough, given all the parent walking.
 
> If so, can we compute this in the content process like you're doing for the
> other values, like the documentURI, charset, etc?

We could.  The main reason I didn't is because setTarget() already has all the necessary walking-up the DOM tree, and isn't causing CPOWS, so it seemed a waste to duplicate it.  Might make sense to move all that stuff to content.js, but since it doesn't appear to be causing CPOWs, it doesn't seem urgent.

Unless it's possible that it is causing CPOWs, but they just aren't being detected to cause the warnings? (I don't know anything about the detection mechanism to know if that's possible)

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Ah, I think we're OK here.

The "contextmenu" is synchronous, and also passes up the event target, which is the item that we're analyzing in that block of code.

That satisfies the conditions for a "safe CPOW usage", since the content process is blocked waiting for a response from the parent, and it passed us the CPOW in the first place, the only overhead here is a bit of IPC traffic.

So I think we're good here. Land away. :)

Comment 20

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Make context-menu open link/frame commands use messages to avoid unsafe CPOW warnings. r=mconley (rebased for landing)

Rebased since bug 1075670 got backed out, and added r=mconley to the commit mesage while I was at it

Small try run at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a39a9f7e5fa0

Full run seemed unnecessary given the greenness of the run in comment 13 and the small nature of the change since then,

Comment 21

[Ian Moody [:Kwan] (UTC+0)]

I need to remember to make use of -u test[platform], those Win7 runs weren't intended/necessary.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/a2672fc5c5e5
https://hg.mozilla.org/integration/fx-team/rev/5532969bd5df

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2672fc5c5e5
https://hg.mozilla.org/mozilla-central/rev/5532969bd5df

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/abb82568a7d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/5049d11255b9

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Why was this uplifted? There was no uplift request for these patches.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Bug 1113431

Comment 27

[Neil Deakin]

This caused regression bug 1159259 I think.