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

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Kwan, Assigned: mconley)

Tracking

(Blocks 1 bug)

Trunk
Firefox 43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox43 fixed)

Details

Attachments

(2 attachments)

+++ 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;
      }
    }
  }

[...]
}
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
  },
Summary: [e10s] "Save Page As..." in remote browser causes unsafe CPOW usage warnings → [e10s] "Save Page|Frame As..." in remote browser causes unsafe CPOW usage warnings
tracking-e10s: m8+ → ---
Whiteboard: [unsafe-cpow-usage]
This is still an issue.
tracking-e10s: --- → ?
Whiteboard: [unsafe-cpow-usage]
Assignee: nobody → mconley
Depends on: 1101100
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.
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.
Bug 1141337 - Make Save Page/Frame As use nsIWebBrowserPersistable to avoid CPOW usage. r?jld
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.
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.
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 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.
Attachment #8647184 - Attachment description: MozReview Request: Bug 1141337 - Make Save Page/Frame As use nsIWebBrowserPersistable to avoid CPOW usage. r?jld → MozReview Request: Bug 1141337 - Update nsIWebBrowserPersistable to allow subframe targeting for Save Frame As. r?jld
Attachment #8647184 - Flags: review?(jld)
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.
Attachment #8647184 - Flags: review?(jld)
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.
Attachment #8647184 - Flags: review?(jld)
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 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. (-:
Attachment #8647184 - Flags: review?(jld)
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!
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb24ca03ed2d439b2059d1151d090077f2b508
changeset:  a4fb24ca03ed2d439b2059d1151d090077f2b508
user:       Mike Conley <mconley@mozilla.com>
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.
https://reviewboard.mozilla.org/r/15921/#review14397

> Do we still need this #include?

Nope! Good eyes. Removed.
https://hg.mozilla.org/mozilla-central/rev/a4fb24ca03ed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.