1134585 - [e10s] "View Selection/MathML Source" 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 in an e10s window, optionally one with MathML
2) Select some page text/find some MathML
2) Right-click on the page/MathML, and choose "View Selection/MathML Source"

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

In browser/base/content/nsContextMenu.js:

  // View Partial Source
  viewPartialSource: function(aContext) {
    var focusedWindow = document.commandDispatcher.focusedWindow;
    if (focusedWindow == window)
      focusedWindow = gBrowser.selectedBrowser.contentWindowAsCPOW;

    var docCharset = null;
    if (focusedWindow)
      docCharset = "charset=" + focusedWindow.document.characterSet; <-- Causes CPOW warning

    // "View Selection Source" and others such as "View MathML Source"
    // are mutually exclusive, with the precedence given to the selection
    // when there is one
    var reference = null;
    if (aContext == "selection")
      reference = focusedWindow.getSelection(); <-- Causes CPOW warning [selection]
    else if (aContext == "mathml")
      reference = this.target;
    else
      throw "not reached";

    // unused (and play nice for fragments generated via XSLT too)
    var docUrl = null;
    window.openDialog("chrome://global/content/viewPartialSource.xul",
                      "_blank", "scrollbars,resizable,chrome,dialog=no",
                      docUrl, docCharset, reference, aContext);
  },

in toolkit/components/viewsource/content/viewPartialSource.js

[far more than I care to list right now, at least 31 distinct source lines, just selecting <a>text</a>, and even more console messages due to loops.  It gets distinctly worse the more complicated the source of the selection/with MathML]

Comment 1

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

This is still around.

Comment 2

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

Neil is taking this one after a rock-paper-scissors battle.

Comment 3

[Neil Deakin]

Attachment: View Partial Selection Changes

Also changes tests that were affected by this.

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

Comment 4

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

Comment on attachment 8640009 [details] [diff] [review]
View Partial Selection Changes

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

Ok, I've reviewed a little over half of this. This looks great, for the most part - just some minor suggestions.

I haven't really reviewed the tests, nor the big swatch of copy-paste that got put into browser-content.js. I'll do that tomorrow.

::: toolkit/components/viewsource/ViewSourceBrowser.jsm
@@ +218,5 @@
>        throw new Error("View source browser's remoteness mismatch");
>      }
>    },
>  
> +  loadViewSourceFromSelection(uri, drawSelection, baseURI) {

Let's get a docstring for this function.

::: toolkit/components/viewsource/content/viewSource-content.js
@@ +758,5 @@
>                                                   [result.line, result.col], 2);
>      sendAsyncMessage("ViewSource:UpdateStatus", { label });
>    },
>  
> +  viewSourceWithSelection(uri, drawSelection, baseURI)

docstring for this method please.

@@ +763,5 @@
> +  {
> +    this.needsDrawSelection = drawSelection;
> +
> +    // all our content is held by the data:URI and URIs are internally stored as utf-8 (see nsIURI.idl)
> +    var loadFlags = Components.interfaces.nsIWebNavigation.LOAD_FLAGS_NONE;

let, not var. Also, we have access to Ci here.

::: toolkit/components/viewsource/content/viewSource.js
@@ +296,5 @@
>      if (!window.arguments[0]) {
>        return;
>      }
>  
> +    if (window.arguments.length >= 4 && window.arguments[3] == "partial") {

With my suggestion for viewSourceUtils, we can put this under the deprecated API check, and then check for args.partial.

::: toolkit/components/viewsource/content/viewSourceUtils.js
@@ +98,2 @@
>     * @param aViewSourceInBrowser
>     *        The browser to display the view source in.

I know you didn't add this comment, but with the aGetBrowserFn thing we've got going on, it's actually not clear whether or not this is the browser that we're supposed to view source in, or the browser we want to view the source _from_.

Can you please update this to be:

"The browser to display the source from."

@@ +105,2 @@
>     */
> +  viewPartialSourceInBrowser: function(aViewSourceInBrowser, aTarget, aGetBrowserFn) {

Why do we want to pass in a function that produces a browser instead of the browser itself? It's probably a simpler API to just have the caller pass in the browser, or null if they want to open in a new window.

@@ +117,5 @@
> +        viewSourceBrowser.loadViewSourceFromSelection(message.data.uri, message.data.drawSelection,
> +                                                      message.data.baseURI);
> +      }
> +      else {
> +        // unused (and play nice for fragments generated via XSLT too)

I don't think this is necessarily unused - it's just unused by Firefox. As this is toolkit code, I suspect there are other apps (I'm looking at you Thunderbird and SeaMonkey) that might still use this path. So I think "unused" is erroneous.

@@ +118,5 @@
> +                                                      message.data.baseURI);
> +      }
> +      else {
> +        // unused (and play nice for fragments generated via XSLT too)
> +        var docUrl = null;

let, not var.

@@ +121,5 @@
> +        // unused (and play nice for fragments generated via XSLT too)
> +        var docUrl = null;
> +        window.openDialog("chrome://global/content/viewPartialSource.xul",
> +                          "_blank", "scrollbars,resizable,chrome,dialog=no",
> +                          message.data.uri, message.data.drawSelection,

This is going to use the old-school arguments mechanism, which puts us into the "deprecated API" function in viewSource.js. Since we're already changing the signature of what viewPartialSource.xul accepts as arguments, we might as well go with the pattern we set up for viewSource.xul.

So instead of passing these individually, can you pass them as an object? Like:

window.openDialog("chrome://global/content/viewPartialSource.xul",
                  "_blank", "scrollbars,resizable,chrome,dialog=no",
                  {
                    URL: message.data.uri,
                    drawSelection: message.data.drawSelection,
                    baseURI: message.data.baseURI,
                    partial: true,
                  });

That's more in line with how we treat viewSource.xul - I think the consistency is valuable.

@@ +126,5 @@
> +                          message.data.baseURI, "partial");
> +      }
> +    });
> +
> +    mm.sendAsyncMessage("ViewSource:GetSelection", { }, { target: aTarget });

ViewSource:GetSelection and ViewSource:GotSelection, are a bit close for me. Can we do:

ViewSource:GetSelection and ViewSource:GetSelection:Done?

::: toolkit/components/viewsource/test/browser/browser_contextmenu.js
@@ +57,5 @@
> +  }
> +
> +  is(items[0], "view-source:http://example.com/", "Link has correct href");
> +  is(items[1], "mailto:[e-mail]", "Link has correct href");
> +  

Nit - trailing ws

::: toolkit/components/viewsource/test/browser/head.js
@@ +40,5 @@
>    });
>  }
>  
> +function* openViewPartialSourceWindow(aURI, aCSSSelector) {
> +  var contentAreaContextMenu = document.getElementById("contentAreaContextMenu");

Please use let not var.

@@ +48,5 @@
> +  yield popupShownPromise;
> +
> +  let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser, null);
> +
> +  document.getElementById("context-viewpartialsource-selection").click();

Shouldn't clicking on this item automatically hide the context menu? Why are you manually doing it?

@@ +54,5 @@
> +  let popupHiddenPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popuphidden");
> +  contentAreaContextMenu.hidePopup();
> +  yield popupHiddenPromise;
> +
> +  return (yield newTabPromise);

I think it's more common to just return the newTabPromise.

@@ +78,2 @@
>  
> +  let newtab = yield* openViewPartialSourceWindow(aURI, aCSSSelector);

I have never seen yield* before. Can you explain it to me?

@@ +80,5 @@
> +
> +  // Wait until the source has been loaded.
> +  yield new Promise(resolve => {
> +    let mm = newtab.linkedBrowser.messageManager;
> +    mm.addMessageListener("ViewSource:SourceLoaded", function selectionDrawn() {

Is there a risk of ViewSource:SourceLoaded being fired before we have a chance to set this message listener?

::: toolkit/content/browser-content.js
@@ +730,5 @@
> +    addMessageListener("ViewSource:GetSelection", this);
> +  },
> +
> +  receiveMessage: function(message) {
> +    switch(message.name) {

It doesn't make a whole lot of sense to use a switch with just a single case - might as well just do an if.

@@ +736,5 @@
> +        let selectionDetails;
> +        try {
> +          selectionDetails = message.objects.target ? this.getMathMLSelection(message.objects.target)
> +                                                    : this.getSelection();
> +        } finally {

Good - I'm glad we've got some guarantees that we'll send back ViewSource:GotSelection.

Comment 5

[Neil Deakin]

> Why do we want to pass in a function that produces a browser instead of the
> browser itself? It's probably a simpler API to just have the caller pass in
> the browser, or null if they want to open in a new window.

The browser doesn't exist when the function is called. It is a browser for a new tab opened after the selection has been retrieved. Opening the new tab earlier will change the focus and could cause the selection to be changed or cleared.


> Shouldn't clicking on this item automatically hide the context menu? Why are
> you manually doing it?

The click() method only sends the dom events 'mousedown', 'mouseup' and 'click', so any behaviour not done though these events (mostly default handling in EventStateManager/PresShell/nsFrames) doesn't happen.

I just copied this code from another test though; I can change it to use synthesizeMouse instead.


> > +  return (yield newTabPromise);
> 
> I think it's more common to just return the newTabPromise.

I want to return the result of the promise (the new tab) not the promise. The test fails otherwise.


> > +  let newtab = yield* openViewPartialSourceWindow(aURI, aCSSSelector);
> 
> I have never seen yield* before. Can you explain it to me?

It is an delegating yield. The openViewPartialSourceWindow is a generator that yields several times. 'yield' would yield only the first value. 'yield*' calls
it repeatedly until it returns, yielding the return value. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield*

It seems to work ok without yield* though perhaps through the magic of promises/tasks.


> Is there a risk of ViewSource:SourceLoaded being fired before we have a
> chance to set this message listener?


Possibly, but I need to add a message listener to a new tab.

Comment 6

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

Comment on attachment 8640009 [details] [diff] [review]
View Partial Selection Changes

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

Finished my review. This looks great - just a few suggestions for the tests.

::: toolkit/components/viewsource/test/browser/browser_bug464222.js
@@ +1,3 @@
>  const source = "http://example.com/browser/toolkit/components/viewsource/test/browser/file_bug464222.html";
>  
> +add_task(function *() {

Glad to see more usage of add_task!

::: toolkit/components/viewsource/test/browser/browser_contextmenu.js
@@ +10,5 @@
> +add_task(function *() {
> +  let newWindow = yield loadViewSourceWindow(source);
> +  yield SimpleTest.promiseFocus(newWindow);
> +
> +  yield* onViewSourceWindowOpen(newWindow, false);

Okay, I understand how yield* works here now, but I'm a bit worried that this adds some cognitive overhead to reading and understanding these tests.

For someone modifying these tests, or using the helper functions in head.js, how do they know when they should use yield or yield*? I don't think it's immediately obvious.

A few suggestions:

1) Add documentation for the helper functions to make it clear how they should be used with yield*
2) Use Task.async for each of the helper functions (and the helper functions they depend on) instead, so that we just need to use yield here.

@@ +12,5 @@
> +  yield SimpleTest.promiseFocus(newWindow);
> +
> +  yield* onViewSourceWindowOpen(newWindow, false);
> +
> +  var contextMenu = gViewSourceWindow.document.getElementById("viewSourceContextMenu");

let, not var

@@ +51,5 @@
> +      return [tags[0].href, tags[1].href];
> +    });
> +  }
> +  else {
> +    let tags = gViewSourceWindow.gBrowser.contentDocument.querySelectorAll("a[href]");

We can technically use the ContentTask in both cases, can we not?

@@ +69,3 @@
>  
> +  if (isTab) {
> +    yield ContentTask.spawn(gBrowser.selectedBrowser, { selector: selector }, function* (arg) {

As above - we can use this regardless of whether or not we're viewing the source in a tab, no?

::: toolkit/content/browser-content.js
@@ +1083,5 @@
> +    return str;
> +  }
> +}
> +
> +ViewSelectionSource.init()

Nit - missing semicolon.

Comment 7

[Neil Deakin]

Attachment: View Partial Selection Changes, v2

Comment 8

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

Comment on attachment 8643999 [details] [diff] [review]
View Partial Selection Changes, v2

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

I think this is fine - my only leftover request is that you document the functions you've modified in head.js, with how they should be used within add_task.

::: toolkit/components/viewsource/test/browser/head.js
@@ +68,4 @@
>    registerCleanupFunction(function() {
>      gBrowser.removeTab(tab);
>    });
> +  

Nit - trailing ws

::: toolkit/content/browser-content.js
@@ +1079,5 @@
> +    str = str.replace(/[^\0-\u007f]/g, convertEntity);
> +
> +    return str;
> +  }
> +}

Nit - missing semi-colon.

Comment 9

[Neil Deakin]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/1199c7fed1fe9bb4fb01dac51dce9d63b4949da8
changeset:  1199c7fed1fe9bb4fb01dac51dce9d63b4949da8
user:       Neil Deakin <[e-mail]>
date:       Mon Aug 10 09:42:51 2015 -0400
description:
Bug 1134585, remove cpow usage from view selection source, r=mconley

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/1199c7fed1fe