1141186 - [e10s] "Share (This (Link|Image|Video)|Selection)" in remote browser causes unsafe CPOW usage warning

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

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

+++ This bug was initially created as a clone of Bug #1133577 +++

STR:

0) Install a social service e.g Twitter
1) Visit a website in an e10s window
2) Right-click on pieces of the page and make use of the "Share [...]" context menu commands

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

In browser/base/content/browser-social.js:

  sharePage: function(providerOrigin, graphData, target) {
[...]
    // if this is a share of a selected item, get any microdata
    if (!pageData.microdata && target) {
      messageManager.addMessageListener("PageMetadata:MicrodataResult", _dataFn = (msg) => {
        messageManager.removeMessageListener("PageMetadata:MicrodataResult", _dataFn);
        pageData.microdata = msg.data;
        this.sharePage(providerOrigin, pageData, target);
      });
      gBrowser.selectedBrowser.messageManager.sendAsyncMessage("PageMetadata:GetMicrodata", null, target); <- Causes CPOW warning
      return;
    }
    this.currentShare = pageData;

[...]
  },


Share selection has a couple extra ones, bug 1134769, and:

In browser/base/content/browser.js:

function getBrowserSelection(aCharLen) {
  // selections of more than 150 characters aren't useful
  const kMaxSelectionLen = 150;
  const charLen = Math.min(aCharLen || kMaxSelectionLen, kMaxSelectionLen);

  let [element, focusedWindow] = BrowserUtils.getFocusSync(document);
  var selection = focusedWindow.getSelection().toString(); <- Causes CPOW warning [selection]
[...]
}

which seems trivial to fix.  Instead of calling getBrowserSelection() in the menuitem itself
https://hg.mozilla.org/mozilla-central/file/eab4a81e4457/browser/base/content/browser-context.inc#l315
just use this.textSelected in shareSelect() in nsContextMenu.js

Comment 1

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

Attachment: Fix context-menu Share commands so they don't use unsafe CPOWs

(In reply to Ian Moody [:Kwan] from comment #0)
> which seems trivial to fix.  Instead of calling getBrowserSelection() in the
> menuitem itself
> https://hg.mozilla.org/mozilla-central/file/eab4a81e4457/browser/base/
> content/browser-context.inc#l315
> just use this.textSelected in shareSelect() in nsContextMenu.js
Yep that works, and gets rid of the getFocusSync(), and turns out wrapping that suspiciously-unwrapped target is enough to fix the other.

Comment 2

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

"Share This Page" doesn't seem to trigger any CPOWs.  I first recorded it as not, then I thought I did hit one, but I can't seem to reproduce it, so removing from summary.

Comment 3

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

Comment on attachment 8574898 [details] [diff] [review]
Fix context-menu Share commands so they don't use unsafe CPOWs

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

Looks good - thanks Ian!

Comment 4

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

Attachment: Fix context-menu Share commands so they don't use unsafe CPOWs

Ah, silly me, I missed a sender of the "PageMetadata:GetMicrodata" message
https://hg.mozilla.org/mozilla-central/file/e965a1a534ec/browser/base/content/socialmarks.xml#l166
Test bustage:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=094396e39ebf

Apologies for not catching this before
Test success with this updated patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d06d6f239f24

This also made me realise I missed the existence of the Save Page|Link To context menu commands (since they weren't captured by '#contentAreaContextMenu menuitem').  Save Link To causes a similar CPOW with the use of "target" in the message payload, but unfortunately that one isn't fixed by the {}ing in this patch, so it's beyond my understanding.

Comment 5

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

Comment on attachment 8579272 [details] [diff] [review]
Fix context-menu Share commands so they don't use unsafe CPOWs

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

> This also made me realise I missed the existence of the Save Page|Link To context menu commands (since they weren't captured by '#contentAreaContextMenu menuitem').  Save Link To causes a similar CPOW with the use of "target" in the message payload, but unfortunately that one isn't fixed by the {}ing in this patch, so it's beyond my understanding.

Hm, alright. Is there a bug on file for that already? If not, can you please file one? Thanks!

Comment 6

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

Green try run in comment #4


(In reply to Mike Conley (:mconley) - Catching up on reviews. from comment #5
> Hm, alright. Is there a bug on file for that already? If not, can you please
> file one? Thanks!
Not yet, haven't had time, but it's on my to-do list.

Comment 7

[Carsten Book [:Tomcat]]

Hi, this didn't apply cleanly : patching file browser/base/content/content.js
Hunk #1 FAILED at 1024
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/content.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh share.patch

Comment 8

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

Attachment: Fix context-menu Share commands so they don't use unsafe CPOWs. r=mconley

(In reply to Carsten Book [:Tomcat] from comment #7)
> Hi, this didn't apply cleanly : patching file browser/base/content/content.js
> Hunk #1 FAILED at 1024
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/base/content/content.js.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh share.patch
Ah, apologies, that'll be because of the aMessage->message changes in Bug 1131457
https://hg.mozilla.org/mozilla-central/rev/ad6136c1a4d0

rebased

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/78d053deaba1

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/78d053deaba1

Comment 11

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

(In reply to Ian Moody [:Kwan] from comment #6)
> (In reply to Mike Conley (:mconley) - Catching up on reviews. from comment #5
> > (In reply to Ian Moody [:Kwan] from comment #4)
> > > This also made me realise I missed the existence of the Save Page|Link To
> > > context menu commands (since they weren't captured by
> > > '#contentAreaContextMenu menuitem').  Save Link To causes a similar CPOW
> > > with the use of "target" in the message payload, but unfortunately that one
> > > isn't fixed by the {}ing in this patch, so it's beyond my understanding.
> >
> > Hm, alright. Is there a bug on file for that already? If not, can you please
> > file one? Thanks!
> Not yet, haven't had time, but it's on my to-do list.

Hmm, after testing again, this seems to have been an issue with the profile I was using.  In a fresh one "Save Link To" produces an unsafe CPOW warning before this patch but not after, so {}ing target did fix it.