1134222 - [e10s] "Save Link As..."/"Bookmark This Link" in remote browser causes unsafe CPOW usage warning

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

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

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

STR:

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

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

In browser/base/content/nsContextMenu.js:

  // Save URL of clicked-on link.
  saveLink: function() {
    var doc =  this.target.ownerDocument; <-- Causes CPOW warning
    var linkText;
    // If selected text is found to match valid URL pattern.
    if (this.onPlainTextLink)
      linkText = this.focusedWindow.getSelection().toString().trim();
    else
      linkText = this.linkText();
    urlSecurityCheck(this.linkURL, this.principal);

    this.saveHelper(this.linkURL, linkText, null, true, doc);
  },


in browser/base/content/utilityOverlay.js:

// Gather all descendent text under given document node.
function gatherTextUnder ( root ) 
{
  var text = "";
  var node = root.firstChild; <-- Causes CPOW warning
  var depth = 1;
  while ( node && depth > 0 ) {
    // See if this node is text.
    if ( node.nodeType == Node.TEXT_NODE ) { <-- Causes CPOW warning
      // Add this text to our collection.
      text += " " + node.data; <-- Causes CPOW warning
    } else if ( node instanceof HTMLImageElement) { <-- Causes CPOW warning [complex, image]
      // If it has an "alt" attribute, add that.
      var altText = node.getAttribute( "alt" ); <-- Causes CPOW warning [image]
      if ( altText && altText != "" ) {
        text += " " + altText;
      }
    }
    // Find next node to test.
    // First, see if this node has children.
    if ( node.hasChildNodes() ) { <-- Causes CPOW warning
      // Go to first child.
      node = node.firstChild; <-- Causes CPOW warning [complex]
      depth++;
    } else {
      // No children, try next sibling (or parent next sibling).
      while ( depth > 0 && !node.nextSibling ) { <-- Causes CPOW warning
        node = node.parentNode; <-- Causes CPOW warning
        depth--;
      }
      if ( node.nextSibling ) { <-- Causes CPOW warning
        node = node.nextSibling; <-- Causes CPOW warning [complex]
      }
    }
  }
  // Strip leading and tailing whitespace.
  text = text.trim();
  // Compress remaining whitespace.
  text = text.replace( /\s+/g, " " );
  return text;
}

note that the amount of warnings depends on the structure of the link, and I've marked up accordingly
untagged warnings are just caused by <a>text</a>
[complex] tagged warnings are caused by e.g. <a><em>text</em> text</a>
[image] tagged warnings are caused by e.g. <a><img/></a>


Note I might make this situation even worse in bug 850378

Comment 1

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

Changing summary to include "Bookmark This Link" since it is a strict subset of this report, hitting all the same CPOWs in utilityOverlay's gatherTextUnder()

Comment 2

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

CPOW's I missed due to not testing with the right markup:

in browser/base/content/nsContextMenu.js:

  // Get text of link.
  linkText: function() {
    var text = gatherTextUnder(this.link);
    if (!text || !text.match(/\S/)) {
      text = this.link.getAttribute("title"); <- Causes CPOW warning
      if (!text || !text.match(/\S/)) {
        text = this.link.getAttribute("alt"); <- Causes CPOW warning
        if (!text || !text.match(/\S/))
          text = this.linkURL;
      }
    }

    return text;
  },

Comment 3

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

Attachment: Store the value of linkText that is calculated during nsContextMenu init and reuse it later, rather than recalculating it

So it turns out the linkText is already being calculated when initing the menu.  In the plaintext case because it needs the text to even work out if it's a link, and in the actual link case, formatSearchContextItem() does it.
(I thought doing all the tree walking in gatherTextUnder() during init, instead of using messages when needed, would have been too expensive for fixing this, but since it is already being done anyway may as well keep it).

This won't fix all the warnings in the save link case, bug 1140859 is for the rest.

Note to self: whichever of this and bug 850378 lands second will need to be rebased on top of the other.

Comment 4

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

Comment on attachment 8574417 [details] [diff] [review]
Store the value of linkText that is calculated during nsContextMenu init and reuse it later, rather than recalculating it

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

This makes sense to me. Glad to see even more unsafe CPOW usage warning reductions! You're on a roll, Ian!

Thank so much.

Comment 5

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

Green linux64 try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc36d24e29ae

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1fe96992c3b5

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/1fe96992c3b5

Comment 8

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

Attachment: MozReview Request: bz://1134222/Kwan

/r/6981 - Bug 1134222 - Save a reference to the ownerDocument during the sync contextmenu message, and use it for saveLink()

Pull down this commit:

hg pull -r 50793a51ef72d275171383c01fc0117f7f455567 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8591205 [details]
MozReview Request: bz://1134222/Kwan

https://reviewboard.mozilla.org/r/6979/#review5787

Thanks, this looks good to me.

Comment 10

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

Cool, with this patch all the remaining work to get Save Link safe is covered by bug 1140859, which I have a patch for.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67fcd4cdbffb
1 intermittent (bug 961215)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdc56a9e357c

Comment 13

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

Bug 1140859 has fixed the remainder.

Comment 14

[Mike Kaply [:mkaply]]

This patch did a very bad thing which is change something that was a function to a string.

Any add-on that used linkText now needs:

+    // linkText changed from a function to a string
+    // in Firefox 39. Check for both.
+    if ("getLinkText" in gContextMenu) {
+      title = gContextMenu.linkText;
+    } else {
+      title = gContextMenu.linkText();
+    }

The string should have had a new variable name, not reused the preexisting function name.

Comment 15

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

(In reply to Mike Kaply [:mkaply] from comment #14)
> This patch did a very bad thing which is change something that was a
> function to a string.
> 
> Any add-on that used linkText now needs:
> 
> +    // linkText changed from a function to a string
> +    // in Firefox 39. Check for both.
> +    if ("getLinkText" in gContextMenu) {
> +      title = gContextMenu.linkText;
> +    } else {
> +      title = gContextMenu.linkText();
> +    }
> 
> The string should have had a new variable name, not reused the preexisting
> function name.
Oh damn it, sorry, I wasn't even thinking of addon compat.  As 39's only in Aurora is it worth changing the variable name, or would that just cause even more problems?

Comment 16

[Mike Kaply [:mkaply]]

Jorge:

Can you do a quick search and see if any add-ons use linkText?

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

I see about 100 add-ons on MXR using this function, so it's definitely a compatibility issue. I think the variable name should be changed, as Mike suggested, and I would like some clarification as to whether this change is being introduced in 39 or 40.

Comment 18

[Mike Kaply [:mkaply]]

We definitely would want to back to using the function and changing the new variable name then. We'll fix our code.

Comment 19

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

Hey Ian,

Feel like filing a new bug to switch the variable name?

Comment 20

[Ben Bucksch (:BenB)]

Thanks, guys, for keeping addon compat here!

Comment 21

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

(In reply to Jorge Villalobos [:jorgev] from comment #17)
> I see about 100 add-ons on MXR using this function, so it's definitely a
> compatibility issue. I think the variable name should be changed, as Mike
> suggested, and I would like some clarification as to whether this change is
> being introduced in 39 or 40.
The issue is introduced in 39 from this landing:
(In reply to Wes Kocher (:KWierso) from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/1fe96992c3b5


(In reply to Mike Conley (:mconley) - Needinfo me! from comment #19)
> Hey Ian,
> 
> Feel like filing a new bug to switch the variable name?
Done, bug 1160339, and given you the r?.

Comment 23

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

Attachment: MozReview Request: Bug 1134222 - Save a reference to the ownerDocument during the sync contextmenu message, and use it for saveLink()