Closed Bug 1058251 Opened 10 years ago Closed 10 years ago

[e10s] File > Save Page As… doesn't work in e10s tabs

Categories

(Firefox :: General, defect, P2)

defect
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.3
Tracking Status
e10s m4+ ---

People

(Reporter: jld, Assigned: jld)

References

Details

(Keywords: dogfood, Whiteboard: [testday-20151002])

Attachments

(2 files)

STR: with a Linux build, with the patches from bug 903022 (currently on inbound) and browser.tabs.remote.autostart set to true, try to use Save Page As… (either from the File menu or with the Ctrl-s shortcut.

Expected: dialog box appears.

Actual: no GUI effect, and "JavaScript error: chrome://browser/content/browser.xul, line 1: window.content is null" printed.

Note that the Save Link As… context menu item does work; it's just Save Page As… that fails.  This also doesn't reproduce in a non-e10s window in the same browser.
Assignee: nobody → jmathies
tracking-e10s: --- → +
I've also reproduced this on Mac OS X.
Priority: -- → P2
OS: Linux → All
Technically this isn't all, it's mac and linux. Windows doesn't see the problem for some reason. I'm guessing some sort of code path difference in front end script. Haven't had a chance to investigate. If anyone with a mac cares to debug, please feel free.
So I can do this to browser/base/content/browser-sets.inc:

> -    <command id="Browser:SavePage" oncommand="saveDocument(window.content.document);"/>
> +    <command id="Browser:SavePage" oncommand="saveDocument(window.gBrowser.selectedBrowser.contentDocumentAsCPOW);"/>

That yields the dialog box (and instantiates nsWebBrowserPersist in the parent process), but the attempt to actually create the files fails.
(In reply to Jim Mathies [:jimm] from comment #2)
> Technically this isn't all, it's mac and linux. Windows doesn't see the
> problem for some reason. I'm guessing some sort of code path difference in
> front end script. Haven't had a chance to investigate. If anyone with a mac
> cares to debug, please feel free.

I was wrong about this, I must have been testing in a non-autostart browser. my mistake.
(In reply to Jed Davis [:jld] from comment #3)
> So I can do this to browser/base/content/browser-sets.inc:
> 
> > -    <command id="Browser:SavePage" oncommand="saveDocument(window.content.document);"/>
> > +    <command id="Browser:SavePage" oncommand="saveDocument(window.gBrowser.selectedBrowser.contentDocumentAsCPOW);"/>
> 
> That yields the dialog box (and instantiates nsWebBrowserPersist in the
> parent process), but the attempt to actually create the files fails.

With this change, in the console I see - 

[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIWebBrowserPersist.saveDocument]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 415"  data: no]
Somewhere in here, that cpow is breaking things - 

http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp#380

Fixing this might be tricky. We can't shuffle the request off the child since sandboxing will break access to the file system. Packing up the page in the child, then shipping that over to the parent is fraught with security issues. Maybe the chrome process should handle loading the page in the background and saving it out so we know we get the document we want and don't have to go through a potentially compromised child.
(In reply to Jim Mathies [:jimm] from comment #6)
> Fixing this might be tricky. We can't shuffle the request off the child
> since sandboxing will break access to the file system. Packing up the page
> in the child, then shipping that over to the parent is fraught with security
> issues. Maybe the chrome process should handle loading the page in the
> background and saving it out so we know we get the document we want and
> don't have to go through a potentially compromised child.

Making the parent validate that the filenames it's passed don't have any unexpected metacharacters or magic strings requires some effort, but it's not intractable (and there may already be code in Gecko for this).

In contrast, interpreting the page in the parent process would expose the entire attack surface of the browser to the page being saved.

Yes, in the absence of [site isolation] (or even the process-per-tab model used on B2G), there might be cases where the child process was persistently compromised by some other site, but the current page isn't malicious (and hasn't been navigated to something else while the user wasn't paying attention, etc.), so it's possible that it's an immensely larger attack surface exposed to a smaller set of attackers… but I'm not convinced it's a good tradeoff.

[site isolation]: http://www.chromium.org/developers/design-documents/site-isolation
Hardware: x86_64 → All
Summary: [e10s] File > Save Page As… doesn't work in e10s tabs on Linux → [e10s] File > Save Page As… doesn't work in e10s tabs
Request re-triage, this should get into a milestone.
Assignee: jmathies → nobody
Keywords: dogfood
Assignee: nobody → mrbkap
Keywords: dogfood
Flags: qe-verify?
Flags: firefox-backlog+
I'm experiencing this same issue on Windows 8.1 x64, with latest Nightly 36.0a1 (2014-11-09)

Right click on page and "Save page as" does work, but clicking Ctrl+S gives error "TypeError: window.content is null browser.xul:1:0" in console.

Using "Save page" from menu gives different error in console
"
[CustomizableUI]" TypeError: win.content is null
CustomizableWidgets<.onCommand@resource:///modules/CustomizableWidgets.jsm:338:9
CustomizableUIInternal.handleWidgetCommand@resource://app/modules/CustomizableUI.jsm:1362:11
 CustomizableUI.jsm:174
"
Hey tim, curious if you have some ideas since you're working on sandboxing. How might we handle this situation where we have page content in a sandboxed child, and we want to save that out to disk?
Flags: needinfo?(tabraldes)
This is Bob's turf :)

ISTM that we want to avoid interpreting pages/sites in the parent process. I would expect us to create or already have an API that allows the child process to specify some output bytes/files that need to be written, and for the parent process to prompt the user for an output directory. The files might be compromised and could conceivably attack some other application upon launch, but at least in Firefox they can only ever be opened in a sandboxed child process.
Flags: needinfo?(tabraldes) → needinfo?(bobowencode)
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #15)
> This is Bob's turf :)
> 
> ISTM that we want to avoid interpreting pages/sites in the parent process. I
> would expect us to create or already have an API that allows the child
> process to specify some output bytes/files that need to be written, and for
> the parent process to prompt the user for an output directory. The files
> might be compromised and could conceivably attack some other application
> upon launch, but at least in Firefox they can only ever be opened in a
> sandboxed child process.

Yeah, I think saving to a file will have to be done through the parent process.
Or at the very least (on Windows) the parent will have to create the file and pass the handle to the child process.
Not quite sure how we'd achieve this second option with the sandboxing code.

Once we get to low integrity level, the content process will only be able to create files in a temporary low integrity directory.
Even this is a stopgap until we can remove all file access from the child process.
Then we might be able to get to an untrusted integrity level, assuming that other things don't block us.
This is the level that Chrome uses for its renderer processes.

Looking at Chrome with procmon while doing a Ctrl-S to save the page, it looks like all the file access is done through the parent (broker) process.

Maybe we can look at how Chromium handles this?

Someone else might be more familiar with this part of the Chromium code base, but I'll leave the needinfo and I'll try and have a look when I get a chance.
(In reply to Bob Owen (:bobowen) from comment #16)
> Or at the very least (on Windows) the parent will have to create the file
> and pass the handle to the child process.
> Not quite sure how we'd achieve this second option with the sandboxing code.

We're already doing this for a few things.  See also PCycleCollectWithLogs, PContent::OpenAnonymousTemporaryFile, and the DMD case of PMemoryReportRequest.  The IPC protocol we need here would be a little more complicated than any of those, but should be doable.
Points: --- → 8
(Taking the bug after checking with the previous assignee on IRC.)

The more I look at nsWebBrowserPersist and contentAreaUtils, the less I like the idea of trying to remote the file access and the nsIWebProgressListener.  What SaveDocument needs from its document might work better: an object that knows its URL/type/encoding, can extract URLs(/types?) for other items it references, and can write out its contents with a given URL translation map.  Put another way: the DOM walking happens in the child; the rest of it (including decisions about what filenames to use and interacting with the download progress UI) stays in the parent.

Also, nsWebBrowserPersist::SaveURI seems to work fine, since it's just reading from one URL and writing to another.  In particular, the failed attempt in comment #3 works in cases other than “Web Page, complete” (non-DOM content like images or text files, or a web page saved as “(X)HTML only”).  This also works, though it lacks some of the filename-suggestion heuristics:

  let doc = window.gBrowser.selectedBrowser.contentDocumentAsCPOW;
  saveURL(doc.location, doc.title, null, false, false, doc.referrer, doc)

There may be a better way to do that.  In the e10s case (== the case where window.content is null?), this strikes me as being an improvement over a “Save As…” that does nothing, and could be worth checking in as a stopgap.
Assignee: mrbkap → jld
Flags: needinfo?(bobowencode)
This makes “Save As” work for e10s, except that it saves web pages as received from the server instead of serializing the current DOM.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e8f07facc11

Now that e10s is enabled by default on Nightly, is it worth trying to check in a workaround like this to make this papercut less sharp, until there's a more complete fix?
Attachment #8521622 - Flags: feedback?(paolo.mozmail)
(In reply to Bob Owen (:bobowen) from comment #16)
> Maybe we can look at how Chromium handles this?

It turns out to be a lot like what I proposed in comment #18: the browser sends the renderer ViewMsg_GetAllSavableResourceLinksForCurrentPage to get the referenced URLs, then ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks to get the serialized adjusted content.
this is still a problem in 2014-11-15 builds on ubuntu x86_64 nightly builds
Comment on attachment 8521622 [details] [diff] [review]
Workaround: use CPOWs and disable DOM-traversal saving for e10s.

Sorry for the delay! I didn't have time to look into this properly, but the approach of allowing plain file saves while disallowing complete web page saves seems good to me, if the "if (aSaveMode != SAVEMODE_FILEONLY) {" line and related change means that we don't suggest "Web page, complete" in the file type dropdown.
Attachment #8521622 - Flags: feedback?(paolo.mozmail) → feedback+
Comment on attachment 8521622 [details] [diff] [review]
Workaround: use CPOWs and disable DOM-traversal saving for e10s.

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

We're missing the "Save Page" toolbar button case. It's defined in CustomizableWidgets.jsm; look for save-page-button.
(In reply to :Paolo Amadini from comment #23)
> Sorry for the delay! I didn't have time to look into this properly, but the
> approach of allowing plain file saves while disallowing complete web page
> saves seems good to me, if the "if (aSaveMode != SAVEMODE_FILEONLY) {" line
> and related change means that we don't suggest "Web page, complete" in the
> file type dropdown.

That's correct; the dropdown is just “HTML document” or “XHTML page” or something along those lines, plus “All Files”.
Updated.  Are there any tests that should be un-disabled for e10s?
Attachment #8524764 - Flags: review?(mconley)
Comment on attachment 8524764 [details] [diff] [review]
Workaround: use CPOWs and disable DOM-traversal saving for e10s. [v2]

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

This looks good to me. I took a quick sweep around our tests, and I couldn't find any that could be enabled with just this fix.

We've got people actively working on these tests though, so if this makes it easier to enable some down the road, well, bonus. :)

Thanks!
Attachment #8524764 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/33425fc431a5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Iteration: --- → 36.3
N.B.: The “Points” field might no longer be accurate, now that remoting nsWebBrowserPersist has been broken out into bug 1101100.
QA Whiteboard: [good first verify][verify in Nightly only]
Flags: qe-verify? → qe-verify-
Is there a bug raised for saving a page as "Web Page, complete" using DOM-traversal in e10s windows?
(In reply to dw-dev from comment #32)
> Is there a bug raised for saving a page as "Web Page, complete" using
> DOM-traversal in e10s windows?

Yes: bug 1101100.
I have reproduced the bug with Firefox Nightly 34.0a1 (20140825030205) on Windows 7 64 Bit.

Verified the fix in latest Firefox Nightly 44.0a1 (20151001030236)

UA: 
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0
Whiteboard: [testday-20151002]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.