Closed Bug 1133577 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
e10s m8+ ---
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: mconley, Assigned: Kwan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

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
    });
  },
See Also: → 1134227
Attached patch WIP patch (obsolete) — Splinter Review
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.
Blocks: 1134227
Depends on: 1134708
See Also: 1134227
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.
Attachment #8567156 - Attachment is obsolete: true
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
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Attachment #8567387 - Flags: review?(mconley)
Blocks: 1134708
No longer depends on: 1134708
(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.
Blocks: 1135619
Blocks: 1135934
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
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?
Let me see if I can reproduce this locally, one sec...
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.
Let me know if you need any help with that.
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.
Flags: needinfo?(mconley)
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
Attachment #8567387 - Attachment is obsolete: true
Attachment #8567387 - Flags: review?(mconley)
Attachment #8571662 - Flags: review?(mconley)
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
Attachment #8570591 - Attachment is obsolete: true
Attachment #8571664 - Flags: review?(mconley)
Blocks: 1134391
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.
(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
Attachment #8571662 - Attachment is obsolete: true
Attachment #8571662 - Flags: review?(mconley)
Attachment #8571953 - Flags: review?(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!
Attachment #8571664 - Flags: review?(mconley) → review+
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?
Attachment #8571953 - Flags: review?(mconley) → review+
Flags: needinfo?(mconley)
(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)
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. :)
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,
Attachment #8571953 - Attachment is obsolete: true
I need to remember to make use of -u test[platform], those Win7 runs weren't intended/necessary.
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/a2672fc5c5e5
https://hg.mozilla.org/mozilla-central/rev/5532969bd5df
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Why was this uplifted? There was no uplift request for these patches.
Flags: needinfo?(ryanvm)
Bug 1113431
Flags: needinfo?(ryanvm)
This caused regression bug 1159259 I think.
Depends on: 1195741
Depends on: 1409248
You need to log in before you can comment on or make changes to this bug.