1141337 - [e10s] "Save Page|Frame As..." in remote browser causes unsafe CPOW usage warnings

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

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

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

STR:

1) Visit a site in an e10s window
2) Right-click on the page, and choose "Save Page As..."

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

In toolkit/content/widgets/remote-browser.xml:

      <property name="contentDocumentAsCPOW" <- Causes CPOW warning
                onget="return this.contentWindowAsCPOW ? this.contentWindowAsCPOW.document : null"
                readonly="true"/>


In toolkit/content/contentAreaUtils.js

function saveDocument(aDocument, aSkipPrompt)
{
  if (!aDocument)
    throw "Must have a document when calling saveDocument";

  // We want to use cached data because the document is currently visible.
  var ifreq =
    aDocument.defaultView <- Causes CPOW warning
             .QueryInterface(Components.interfaces.nsIInterfaceRequestor);

  var contentDisposition = null;
  try {
    contentDisposition =
      ifreq.getInterface(Components.interfaces.nsIDOMWindowUtils) <- Causes CPOW warning
           .getDocumentMetadata("content-disposition");
  } catch (ex) {
    // Failure to get a content-disposition is ok
  }

  var cacheKey = null;
  try {
    cacheKey =
      ifreq.getInterface(Components.interfaces.nsIWebNavigation) <- Causes CPOW warning
           .QueryInterface(Components.interfaces.nsIWebPageDescriptor);
  } catch (ex) {
    // We might not find it in the cache.  Oh, well.
  }

  if (cacheKey && Components.utils.isCrossProcessWrapper(cacheKey)) {
    // Don't use a cache key from another process. See bug 1128050.
    cacheKey = null;
  }

  internalSave(aDocument.location.href, aDocument, null, contentDisposition, <- Causes CPOW warning
               aDocument.contentType, false, null, null, <- Causes CPOW warning
               aDocument.referrer ? makeURI(aDocument.referrer) : null, <- Causes CPOW warning
               aDocument, aSkipPrompt, cacheKey);
}

[...]

function internalSave(aURL, aDocument, aDefaultFileName, aContentDisposition,
                      aContentType, aShouldBypassCache, aFilePickerTitleKey,
                      aChosenData, aReferrer, aInitiatingDocument, aSkipPrompt,
                      aCacheKey)
{
[...]

    continueSave();
  } else {
    var charset = null;
    if (aDocument)
      charset = aDocument.characterSet; <- Causes CPOW warning
    else if (aReferrer)
      charset = aReferrer.originCharset;
    var fileInfo = new FileInfo(aDefaultFileName);
    initFileInfo(fileInfo, aURL, charset, aDocument,
                 aContentType, aContentDisposition);
    sourceURI = fileInfo.uri;

[...]
  }

[...]

function getDefaultFileName(aDefaultFileName, aURI, aDocument,
                            aContentDisposition)
{
[...]

  let docTitle;
  if (aDocument) {
    // If the document looks like HTML or XML, try to use its original title.
    docTitle = validateFileName(aDocument.title).trim(); <- Causes CPOW warning
    if (docTitle) {
      let contentType = aDocument.contentType; <- Causes CPOW warning
      if (contentType == "application/xhtml+xml" ||
          contentType == "application/xml" ||
          contentType == "image/svg+xml" ||
          contentType == "text/html" ||
          contentType == "text/xml") {
        // 2) Use the document title
        return docTitle;
      }
    }
  }

[...]
}

Comment 1

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

Also "Save Frame As..." has all the same plus

In browser/base/content/nsContentMenu.js:

  // Save URL of clicked-on frame.
  saveFrame: function () {
    saveDocument(this.target.ownerDocument); <- Causes CPOW warning
  },

Comment 2

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

This is still an issue.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

The current work in progress for bug 1101100 should take care of this for “Save Page As”, but “Save Frame As” is going to be harder to fix.

Comment 4

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

I do plan on making use of bug 1101100 to solve this - though I'll probably need to augment the changes in the patch to accept an outer window ID in the startPersistence method of nsIWebBrowserPersistable.

Comment 5

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

Attachment: MozReview Request: Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

Bug 1141337 - Make Save Page/Frame As use nsIWebBrowserPersistable to avoid CPOW usage. r?jld

Comment 6

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

https://reviewboard.mozilla.org/r/15921/#review14205

::: embedding/components/webbrowserpersist/nsIWebBrowserPersistable.idl:28
(Diff revision 1)
> + *        will be fired once the document is ready for persisting.

Add the aOuterWindowID documentation too.

Comment 7

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

https://reviewboard.mozilla.org/r/15921/#review14215

::: dom/ipc/TabChild.cpp:3134
(Diff revision 1)
> +    if (window) {
> +      doc = window->GetDoc();
> +    }

Should probably ensure that this nsIDOMWindow is in the tree of this TabChild.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

https://reviewboard.mozilla.org/r/15921/#review14217

r=me with the changes noted.

::: dom/ipc/PBrowser.ipdl:124
(Diff revision 1)
> -    PWebBrowserPersistDocument();
> +    PWebBrowserPersistDocument(uint64_t aOuterWindowID);

This seems like a good place for a comment that the outer window ID is ignored when sent child->parent.  (And maybe also that window ID 0 means the top-level document when parent->child.)

::: dom/ipc/TabChild.cpp:3134
(Diff revision 1)
> +    if (window) {
> +      doc = window->GetDoc();
> +    }

I was also going to ask that, and if there's a good way to deduplicate it with the ancestry check that's already in `nsFrameLoader` for the in-process case.

::: embedding/components/webbrowserpersist/WebBrowserPersistResourcesChild.cpp:40
(Diff revision 1)
> -    if (!grandManager->SendPWebBrowserPersistDocumentConstructor(subActor)) {
> +    // We pass 0 for the WebBrowserPersistDocumentConstructor because we don't

This comment isn't quite right: the outer window ID will be ignored in the child->parent case.  The actor will always be associated with `aSubDocument` because that's the one passed to `WebBrowserPersistDocumentChild::Start` later in this method.

Comment 9

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

Comment on attachment 8647184 [details]
MozReview Request: Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

This alters nsIWebBrowserPersistable so that startPersistence takes an
outerWindowID. This allows us to target a particular subframe from
beneath an nsFrameLoader, which is useful when attempting to Save
Frame As a remote browser.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8647184 [details]
MozReview Request: Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

https://reviewboard.mozilla.org/r/15921/#review14289

::: dom/base/nsFrameLoader.cpp:2911
(Diff revisions 1 - 2)
> -    if (!found) {
> +  if (!targetDoc) {

Should this really silently default to the root document if a window ID is given but doesn't exist (or isn't a descendant)?  I'd expect that to be an error.

::: dom/ipc/TabChild.cpp:3135
(Diff revisions 1 - 2)
>      nsCOMPtr<nsPIDOMWindow> window = nsGlobalWindow::GetOuterWindowWithId(aOuterWindowID);

I still don't really like nsFrameLoader and TabChild duplicating this code; it seems like a good way to wind up with strange unintended differences between in-process and out-of-process.  But I'm not sure where a good place to put the common code would be.  (nsContentUtils?  nsDocument itself?)

::: dom/ipc/TabChild.cpp:3148
(Diff revisions 1 - 2)
>      // Fallback to just grabbing the root document

See the comment in nsFrameLoader.cpp.

Comment 11

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

Comment on attachment 8647184 [details]
MozReview Request: Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

This alters nsIWebBrowserPersistable so that startPersistence takes an
outerWindowID. This allows us to target a particular subframe from
beneath an nsFrameLoader, which is useful when attempting to Save
Frame As a remote browser.

Comment 12

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

https://reviewboard.mozilla.org/r/15921/#review14289

> Should this really silently default to the root document if a window ID is given but doesn't exist (or isn't a descendant)?  I'd expect that to be an error.

Good point. I've altered this so that we send NS_ERROR_NO_CONTENT to the receiver instead.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8647184 [details]
MozReview Request: Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

https://reviewboard.mozilla.org/r/15921/#review14397

::: dom/ipc/TabChild.cpp:102
(Diff revision 3)
> +#include "nsGlobalWindow.h"

Do we still need this #include?

::: toolkit/content/contentAreaUtils.js:145
(Diff revision 3)
> -    // nsresult, but in case of asynchronous failure there isn't
> +      Components.utils.reportError(aError);

I think that will just log the `nsresult` as a decimal integer, but I see you noticed the impending merge conflict with bug 1193903 which will take care of that. (-:

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8647184 [details]
MozReview Request: Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld

https://reviewboard.mozilla.org/r/15921/#review14399

Ship It!

Comment 15

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb24ca03ed2d439b2059d1151d090077f2b508
changeset:  a4fb24ca03ed2d439b2059d1151d090077f2b508
user:       Mike Conley <[e-mail]>
date:       Thu Aug 06 10:44:16 2015 -0400
description:
Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r=jld

This alters nsIWebBrowserPersistable so that startPersistence takes an
outerWindowID. This allows us to target a particular subframe from
beneath an nsFrameLoader, which is useful when attempting to Save
Frame As a remote browser.

Comment 16

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

https://reviewboard.mozilla.org/r/15921/#review14397

> Do we still need this #include?

Nope! Good eyes. Removed.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1141337.html