Closed
Bug 1141337
Opened 9 years ago
Closed 9 years ago
[e10s] "Save Page|Frame As..." in remote browser causes unsafe CPOW usage warnings
Categories
(Firefox :: Menus, defect)
Firefox
Menus
Tracking
()
RESOLVED
FIXED
Firefox 43
People
(Reporter: Kwan, Assigned: mconley)
References
Details
Attachments
(2 files)
+++ 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; } } } [...] }
Reporter | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
tracking-e10s:
m8+ → ---
Whiteboard: [unsafe-cpow-usage]
Assignee | ||
Comment 2•9 years ago
|
||
This is still an issue.
tracking-e10s:
--- → ?
Whiteboard: [unsafe-cpow-usage]
Updated•9 years ago
|
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
Bug 1141337 - Make Save Page/Frame As use nsIWebBrowserPersistable to avoid CPOW usage. r?jld
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8647184 -
Flags: review+
Comment 14•9 years ago
|
||
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!
Assignee | ||
Comment 15•9 years ago
|
||
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.
Assignee | ||
Comment 16•9 years ago
|
||
https://reviewboard.mozilla.org/r/15921/#review14397 > Do we still need this #include? Nope! Good eyes. Removed.
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a4fb24ca03ed
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Assignee | ||
Comment 18•8 years ago
|
||
bugnotes |
http://people.mozilla.org/~mconley2/bugnotes/bug-1141337.html
You need to log in
before you can comment on or make changes to this bug.
Description
•