Splitting bug 1058251; that will cover basic ”Save As” functionality, and this will cover getting the “Complete Document” mode (and by extentsion nsIWebBrowserPersist) working for e10s. +++ This bug was initially created as a clone of Bug #1058251 +++

It's been brought up that this is a common thing for users of Dev Edition, so we might want to bump this up to m7

On Mac Dev Edition, the "Save As" file picker doesn't even display the different options ("Complete Document" being one of them). There are just 2 options ("Firefox.app Document" and "All Files"), and both end up saving the HTML doc only. Not sure if this misbehavior should be filed as a different bug, but it's likely related to this one anyway.

(In reply to André Reinald from comment #3) > On Mac Dev Edition, the "Save As" file picker doesn't even display the > different options ("Complete Document" being one of them). There are just 2 > options ("Firefox.app Document" and "All Files"), and both end up saving the > HTML doc only. Not sure if this misbehavior should be filed as a different > bug, but it's likely related to this one anyway. That looks like the expected result of the bug 1058251 workaround (compare with the result of using Save As after navigating to a JPEG file, for example) so it's something this bug will fix.

I haven't dealt with contentAreaUtils.js yet, so “Save As” still won't do e10s, but nsIWebBrowserPersist::SaveDocument will now accept an nsIFrameLoaderOwner (e.g., gBrowser.selectedBrowser) in place of a local document, and will make things work appropriately for either in-process or out-of-process frames. (Hint: set persistFlags to contain at least 16384, the way contentAreaUtils does, so that gzipped-for-transfer JS/CSS/etc are decompressed before saving.) There aren't a lot of comments yet, but there are a bunch of FIXMEs, and things are kind of messy, and error handling is probably semi-broken, but it seems to at least minimally work.

The basic idea is as in bug 1058251 comment #18 — establish an abstraction for the DOM walking and document encoding with link fixup that can be remoted over IPC, and then do that, and make the file manipulation (and WebProgress) parts talk to it. This requires making some previously sync operations (tree walking, document encoding) async and throwing them in with the already large amount of logic for handling async network downloads and uploads (including the cases for serialized uploads introduced in bug 138662), which is probably the sketchiest part of the current patch. (I also don't entirely like the nsIWordsWordsWordsWords class names I chose, but there may not be a good alternative.)

(In reply to Mike Conley (:mconley) from bug 1128050 comment #12) > What this means is that if a user reaches a page via POST, and then clears > their cache, and then attempts to save the page, the page will probably not > be saved correctly (since the postData won't be resubmitted, which is what > we do in the non-e10s case). > > I think this is strictly better than being unable to save pages reached via > POST at all, and I suspect we can live with this until bug 1101100 fixes > this properly. Well, this is bug 1101100… and now this is my problem, in the case where the document reached by POST isn't being saved by serializing the DOM.

Another problem: “Save Frame As…” in the context menu. This runs in the parent process with a CPOW for the target document. It looks like I can send MessageManager messages with the browser object, but that doesn't help me set up the IPDL actors.

Newer WIP. Wired up to the “Save As” UI elements, and more or less Just Works in the common case of a document with a DOM, at least as long as there are no network errors, and it's not a “Save Frame As”. It doesn't yet handle a few things about the document that contentAreaUtils wants: title, content-disposition, referrer, cache key, and as mentioned postdata (which is the only one that will be actually difficult to deal with). Most of that only matters for non-DOM saving; the title and content-disposition are used to suggest a filename for the document, but it can fall back to the URL basename.

This… seems to work? The nsIIPCSerializableInputStream stuff Just Works, it turns out, so all the document properties contentAreaUtils.js wants are relatively easy to transport over IPC. I've confirmed that a plain text document returned by a POST with a file upload is saved correctly even after clearing the cache (in which case the it's silently re-POSTed, but that was already true). The patch is still light on comments and heavy on FIXME, and still leaves the “Save Frame As…” case without e10s DOM-saving abilities, but no more broken than it was before this.

Hey jld - I saw you and paolo talking during the last night in Whistler. Did he have some suggestions on how to proceed here?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11) > Hey jld - I saw you and paolo talking during the last night in Whistler. Did > he have some suggestions on how to proceed here? Paolo had been working on rewriting nsWebBrowserPersist in JavaScript. Which would definitely help clean up the part of it that's still in nsWebBrowserPersist.cpp after this patch — there's a lot of difficult-to-follow async callback stuff happening there, and having a language that better supports writing that kind of code comprehensibly would help. The DOM traversal (the new nsWebBrowserPersistDocument.cpp) could stay in C++, since it'd be roughly the same code in either language. One possibly useful thing: we may not need to keep the ability to serialize to non-file destinations (but Netscape Composer used it, and Seamonkey Composer may still, for uploading sites?), and that removes a bunch of the complexity — all the code for handling uploads (and the weirdly overloaded purpose of the OnStopRequest method) goes away, and the nsIOutputStream that's currently being clumsily remoted will always be a file instead of sometimes an nsStorageStream (but using NS_OpenAnonymousTemporaryFile also accomplishes that last item). I'm not sure how certain we are about that. Where we left things, I think, is that we can probably land this patch in roughly its current form (with more comments, and more work making sure the nsIWebProgress notifications are still consistent, and maybe redoing the output stream remoting) and leave the other improvements we talked about as followup work.

Still needs more comments, but I *think* the state/status callback stuff is more or less reasonable now.

I ran Try, and I'm getting an xpcshell test failure I don't quite understand: 19:33:12 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js | test_empty_progress - [test_empty_progress : 406] false == true 19:33:12 INFO - /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js -> file:///builds/slave/test/build/tests/xpcshell/tests/toolkit/components/jsdownloads/test/unit/common_test_Download.js:test_empty_progress:406 19:33:12 INFO - self-hosted:next:673 19:33:12 INFO - _run_next_test@/builds/slave/test/build/tests/xpcshell/head.js:1460:9 19:33:12 INFO - do_execute_soon/<.run@/builds/slave/test/build/tests/xpcshell/head.js:653:9 19:33:12 INFO - _do_main@/builds/slave/test/build/tests/xpcshell/head.js:207:5 19:33:12 INFO - _execute_test@/builds/slave/test/build/tests/xpcshell/head.js:513:5 That's this code: /** * Downloads a file with a "Content-Length" of 0 and checks the progress. */ add_task(function test_empty_progress() { let download = yield promiseStartDownload(httpUrl("empty.txt")); [...] do_check_true(download.hasProgress); But I didn't change anything about the progress notifications, and I didn't think I'd changed anything meaningful about the SaveURI case.

paolo: Any thoughts about comment #14? The failing xpcshell runs are in https://treeherder.mozilla.org/#/jobs?repo=try&revision=582652c68767

For application after bug 1179967, and now with comments on the XPIDL and IPDL files, and a little cleanup to some inelegance in the IPDL that it was easier to fix than apologize for. Could use some more comments in the actual code (especially anything to do with actor lifetimes) and there's still the jsdownloads breakage mentioned earlier.

Yet more comments. I might be ready to inflict this on a reviewer at this point, if I can get somewhere with the test failure.

(In reply to Jed Davis [:jld] {UTC-7} from comment #14) > let download = yield promiseStartDownload(httpUrl("empty.txt")); > [...] > do_check_true(download.hasProgress); > > But I didn't change anything about the progress notifications, and I didn't > think I'd changed anything meaningful about the SaveURI case. Nothing comes to my mind, there's evidently a difference but needs to be debugged.

Comment on attachment 8630868 [details] [diff] [review] Work In Progress (2015-07-07 #2) Bill: I'm told you're probably the closest thing to the right reviewer for this, but feel free to hand off parts of it as needed. There's a known test failure with this (see above about jsdownloads) so I don't know that r? is the right flag, but hopefully I won't have to change too much once I find out what's going on there, so most of this would be the same as the final review-ready patch. There are still a couple of FIXME comments left, which are effectively questions for reviewers now.

(In reply to Jed Davis [:jld] {UTC-7} from comment #14) > But I didn't change anything about the progress notifications, and I didn't > think I'd changed anything meaningful about the SaveURI case. Not quite. The OnStateChange callbacks from On{Start,Stop}Request are missing STATE_IS_NETWORK, because I moved that to a separate start/stop pair for the SaveDocument case, and in the process also deleted the code that ensures that it happens in the SaveURI/SaveChannel case. The fix is trivial, but I'll wait for a Try run before declaring victory.

Includes the jsdownloads test fix; excludes bug 1179967 which is on its way to m-c; upgrading f? to r?.

Comment on attachment 8634955 [details] [diff] [review] Big Patch (2015-07-16) This isn't really “work in progress” anymore with an r? on it, I guess. I'm not sure if this is better to leave as one large patch or split into multiple steps — I don't know if anything would be gained by, say, having an intermediate state where the nsIWebBrowserPersistDocument machinery exists and is implemented but nsWebBrowserPersist itself doesn't use it or know how to talk to it. There were a few states like that while I was developing it, so I could do ad-hoc testing in the Web Console and make sure that what I'd written so far more or less worked before building on it, but that may not be as helpful for review or for anyone trying to understand the changes post-landing.

Sorry this is taking me so long. I'm working through a little bit each day. Can we talk about it tomorrow or Friday?

Comment on attachment 8634955 [details] [diff] [review] Big Patch (2015-07-16) Review of attachment 8634955 [details] [diff] [review]: ----------------------------------------------------------------- This looks really nice. It would be nice to get rid of the 'ns' namespacing for at least these new files. ::: dom/base/nsFrameLoader.h @@ +55,5 @@ > > class nsFrameLoader final : public nsIFrameLoader, > public nsStubMutationObserver, > + public mozilla::dom::ipc::MessageManagerCallback, > + public nsIWebBrowserPersistable Maybe move this up near nsIFrameLoader so the interfaces are together? ::: dom/ipc/TabChild.cpp @@ +3579,5 @@ > +PWebBrowserPersistDocumentChild* > +TabChild::AllocPWebBrowserPersistDocumentChild() > +{ > + return new nsWebBrowserPersistDocumentChild(); > + MOZ_CRASH("not implemented yet"); Ahem. ::: dom/ipc/TabParent.cpp @@ +3346,5 @@ > +TabParent::StartPersistence(nsIWebBrowserPersistDocumentReceiver* aRecv) > +{ > + auto* actor = new nsWebBrowserPersistDocumentParent(); > + actor->SetOnReady(aRecv); > + return SendPWebBrowserPersistDocumentConstructor(actor) Right now, for example, we won't delete the actor if this Send call fails, I don't think. ::: embedding/components/webbrowserpersist/PWebBrowserPersistDocument.ipdl @@ +16,5 @@ > +// this structure from the child to the parent before the parent actor > +// is exposed to XPCOM. > +struct WebBrowserPersistDocumentAttrs { > + bool isPrivate; > + nsCString documentURI; I was thinking we should use URIParams here. However, it looks like blob: URIs and the like are not persistable. So I guess there's no need. But can you please comment to that effect? @@ +48,5 @@ > +// lifecycle is a little complicated: the initial document is > +// constructed parent->child, but subdocuments are constructed > +// child->parent and then passed back. Subdocuments aren't subactors, > +// because that would impose a lifetime relationship that doesn't > +// exist in the XIPDL; instead they're all managed by the enclosing XIPDL? ::: embedding/components/webbrowserpersist/nsIWebBrowserPersistDocument.idl @@ +133,5 @@ > + * > + * @param aDocument The document containing the reference. > + * @param aURI The absolute URI spec for the referenced resource. > + */ > + void visitURI(in nsIWebBrowserPersistDocument aDocument, Calling this visitResource seems a little nicer. ::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp @@ +173,5 @@ > + > +NS_IMPL_ISUPPORTS(nsWebBrowserPersist::OnStart, > + nsIWebBrowserPersistDocumentReceiver) > + > +nsWebBrowserPersist::OnStart::OnStart(nsWebBrowserPersist* aParent, I think it would be fine to put constructors like this inline in the class definition. @@ +179,5 @@ > + nsIURI* aDataPath) > +: mParent(aParent) > +, mFile(aFile) > +, mDataPath(aDataPath) > +{ Lots of extra whitespace in this file. @@ +560,5 @@ > + rv = start->OnDocumentReady(doc); > + } else { > + nsCOMPtr<nsIWebBrowserPersistable> pdoc = do_QueryInterface(aDocument); > + if (pdoc) { > + rv = pdoc->StartPersistence(start); This seems overly complex to me. As far as I can tell, SaveDocument only really ever takes an nsDocument or an nsIWebBrowserPersistDocument. In either case, we can get started right away and there's no need for asynchrony. Could we just change the interface so that this function QIs to nsIDOMDocument and then constructs its own nsWebBrowserPersistDocument? @@ +582,5 @@ > + rv = NS_ERROR_FAILURE; > + } > + if (NS_FAILED(rv)) { > + mParent->SendErrorStatusChange(true, rv, nullptr, mParent->mURI); > + mParent->EndDownload(rv); If OnDocumentReady is actually called synchronously from nsDocument, I think we can get two EndDocument calls here. @@ +1783,5 @@ > + mWalkStack.LastElement().swap(toWalk); > + mWalkStack.TruncateLength(mWalkStack.Length() - 1); > + // Bounce this off the event loop to avoid stack overflow. > + // FIXME: is there any non-ugly way to indent this. > + NS_DispatchToCurrentThread(NS_NewRunnableMethodWithArg<StoreCopyPassByRRef<mozilla::UniquePtr<WalkData>>>(this, &nsWebBrowserPersist::SaveDocumentDeferred, mozilla::Move(toWalk))); Break it up into multiple statements? Maybe with a typedef. ::: embedding/components/webbrowserpersist/nsWebBrowserPersist.h @@ +56,5 @@ > > +// FIXME: Is the right set of members still protected vs. private > +// after all these changes? Or does it even matter, given that > +// nothing inherits from this class? (i.e., can I just change > +// protected to private and declare the class final?) Sounds good to me! @@ +76,2 @@ > nsresult SaveDocuments(); > + void FinishSaveDocumentInternal(nsIURI* aFIle, nsIFile* aDataPath); aFile ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocument.cpp @@ +439,5 @@ > +nsresult > +ResourceReader::OnWalkDOMNode(nsIDOMNode* aNode) > +{ > + nsresult rv; > + WS @@ +1267,5 @@ > + MOZ_ASSERT(oldStyleDoc); > + rv = oldStyleDoc->CreateTreeWalker(docAsNode, > + nsIDOMNodeFilter::SHOW_ELEMENT | > + nsIDOMNodeFilter::SHOW_DOCUMENT | > + nsIDOMNodeFilter::SHOW_PROCESSING_INSTRUCTION, Indent ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocument.h @@ +19,5 @@ > + : public nsIWebBrowserPersistDocument > +{ > +public: > + explicit nsWebBrowserPersistDocument(nsIDocument* aDocument); > + Extra space. @@ +24,5 @@ > + const nsCString& GetCharacterSet() const; > + uint32_t GetPersistFlags() const; > + already_AddRefed<nsIURI> GetBaseURI() const; > + > + NS_DECL_ISUPPORTS Given that this object is accessible form JS, I feel like it should be cycle collected. @@ +28,5 @@ > + NS_DECL_ISUPPORTS > + NS_DECL_NSIWEBBROWSERPERSISTDOCUMENT > + > +private: > + nsCOMPtr<nsIDocument> mDocument; // Reference cycles? ...which would address this concern. ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentChild.cpp @@ +23,5 @@ > + > +void > +nsWebBrowserPersistDocumentChild::Start(nsIDocument* aDocument) > +{ > + Start(aDocument ? new nsWebBrowserPersistDocument(aDocument) : nullptr); I think we leak the new nsWBPDocument if Start encounters an error. Can you stash it in an nsRefPtr local before passing it in? @@ +74,5 @@ > +mozilla::PWebBrowserPersistDocumentReadChild* > +nsWebBrowserPersistDocumentChild::AllocPWebBrowserPersistDocumentReadChild() > +{ > + auto* actor = new nsWebBrowserPersistDocumentReadChild(); > + NS_ADDREF(actor); I think it's considered preferable to assign this to an nsRefPtr and then return .forget(). ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentParent.cpp @@ +44,5 @@ > +nsWebBrowserPersistDocumentParent::FireOnReady() > +{ > + MOZ_ASSERT(!WaitingForAttrs()); > + MOZ_ASSERT(mHoldingExtraRef); > + MOZ_ASSERT(mOnReady); Since a misbehaving child can cause this, I'd rather not assert here. Maybe just warn? @@ +246,5 @@ > +nsWebBrowserPersistDocumentParent::SetPersistFlags(uint32_t aFlags) > +{ > + nsresult rv = AccessAttrs(); > + if (NS_SUCCEEDED(rv)) { > + if (!SendSetPersistFlags(aFlags)) { Could ActorDestroy have been called before this? If so, we need to error out. Same for any other Send calls here. ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentParent.h @@ +14,5 @@ > + > +// This class implements nsIWebBrowserPersistDocument for a remote document. > +// See also mozilla::dom::TabParent::StartPersistence. > + > +class nsWebBrowserPersistDocumentParent final I think the lifetimes would be a lot simpler if you split this into two classes. One would be the actor and the other would be the XPCOM object. The actor would have a strong ref (nsRefPtr) to the XPCOM object before initialization finishes. Then it would switch to a weak ref (raw ptr). The XPCOM object's destructor would call Send__delete__ on the actor if it still had a ref. The DeallocPWebBrowserPersistDocumentParent call would delete the actor. I'm hoping this would eliminate the manual addref/release operations in the patch and make it a little less error-prone. ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentWriteChild.cpp @@ +93,5 @@ > + MOZ_RELEASE_ASSERT(NS_IsMainThread(), "Fix this class to be thread-safe."); > + > + // Limit the size of an individual IPC message. > + static const uint32_t kMaxWrite = 65536; > + WS @@ +105,5 @@ > + // It would be nice if this extra copy could be avoided. > + buf.AppendElements(aBuf, toWrite); > + SendWriteData(mozilla::Move(buf)); > + *aWritten += toWrite; > + aBuf += toWrite; Can you use a local var rather than updating aBuf? ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentWriteParent.h @@ +21,5 @@ > + nsIWebBrowserPersistDocument* aDocument, > + nsIOutputStream* aStream, > + nsIWebBrowserPersistWriteCompletion* aFinish); > + virtual ~nsWebBrowserPersistDocumentWriteParent(); > + WS ::: toolkit/content/contentAreaUtils.js @@ +126,4 @@ > aDoc, aSkipPrompt, null); > } > > +function saveFrame(aFrame, aSkipPrompt) aBrowser would be a more descriptive name. @@ +133,5 @@ > + } > + aFrame.QueryInterface(Ci.nsIFrameLoaderOwner) > + .frameLoader > + .QueryInterface(Ci.nsIWebBrowserPersistable) > + .startPersistence(function (document) { Could you do this as two separate statements (get the object and then call startPersistence on it)? @@ +142,1 @@ > function saveDocument(aDocument, aSkipPrompt) Need a comment to say what the expected types of aDocument are. Looks like it's either nsIWebBrowserPersistDocument or nsIDOMDocument? @@ +149,2 @@ > > + if ("contentDisposition" in aDocument && "cacheKey" in aDocument) { Is there any reason not to use instanceof here? It seems like it would be more direct. @@ +150,5 @@ > + if ("contentDisposition" in aDocument && "cacheKey" in aDocument) { > + // nsIWebBrowserPersistDocument exposes these directly. > + contentDisposition = aDocument.contentDisposition; > + cacheKeyInt = aDocument.cacheKey; > + } else if ("defaultView" in aDocument) { Same here. @@ +153,5 @@ > + cacheKeyInt = aDocument.cacheKey; > + } else if ("defaultView" in aDocument) { > + // Otherwise it's an actual nsDocument (and possibly a CPOW). > + // We want to use cached data because the document is currently visible. > + var ifreq = var -> let please. @@ +173,5 @@ > + .currentDescriptor > + .QueryInterface(Components.interfaces.nsISHEntry); > + > + let cacheKey = shEntry.cacheKey > + .QueryInterface(Components.interfaces It probably makes sense to say |const Ci = ...| at the top of this function. @@ +176,5 @@ > + let cacheKey = shEntry.cacheKey > + .QueryInterface(Components.interfaces > + .nsISupportsPRUint32) > + .data; > + // cacheKey might be a CPOW, but numbers aren't wrapped. Under what circumstances will aDocument be a CPOW with your changes? It looks like it happens if we try to save a sub-frame using the context menu. And I guess an add-on could do it. If that's true, please add a comment. @@ +375,4 @@ > let nonCPOWDocument = > aDocument && !Components.utils.isCrossProcessWrapper(aDocument); > > + let isPrivate = "defaultView" in aInitiatingDocument Same instanceof thing here. @@ +955,4 @@ > } > > let docTitle; > + if (aDocument && aDocument.title) { What's this for? It looks like nsIWebBrowserPersistDocument has a title attribute, so it seems like you're not changing behavior here.

(In reply to Bill McCloskey (:billm) from comment #25) > ::: dom/ipc/TabParent.cpp > @@ +3346,5 @@ > > +TabParent::StartPersistence(nsIWebBrowserPersistDocumentReceiver* aRecv) > > +{ > > + auto* actor = new nsWebBrowserPersistDocumentParent(); > > + actor->SetOnReady(aRecv); > > + return SendPWebBrowserPersistDocumentConstructor(actor) > > Right now, for example, we won't delete the actor if this Send call fails, I > don't think. I think we do. From the autogenerated definition of PBrowserParent::SendPWebBrowserPersistDocumentConstructor: > bool sendok__ = (mChannel)->Send(msg__); > if ((!(sendok__))) { > (actor)->DestroySubtree(FailedConstructor); > (actor)->DeallocSubtree(); DestroySubtree -> ActorDestroy -> (deferred) Release -> delete. See also the comment I left on the other direction: > + if (!grandManager->SendPWebBrowserPersistDocumentConstructor(subActor)) { > + // NOTE: subActor is freed at this point. (Except that's in the child, so we might actually exit instead?)

(In reply to Bill McCloskey (:billm) from comment #25) > It would be nice to get rid of the 'ns' namespacing for at least these new > files. So, mozilla::WebBrowserPersistClassName? I remember that I got pushback on mozilla::sandbox (which is why there's a lot of mozilla::SandboxThing), so I'm guessing mozilla::webbrowserpersist::ClassName wouldn't fly? The XPIDL can't be namespaced, and I've seen the occasional mozIThing but given that there's an established nsIThing here it seems better to stick with that for the XPCOM interfaces?

Comment on attachment 8634955 [details] [diff] [review] Big Patch (2015-07-16) Review of attachment 8634955 [details] [diff] [review]: ----------------------------------------------------------------- ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentChild.cpp @@ +74,5 @@ > +mozilla::PWebBrowserPersistDocumentReadChild* > +nsWebBrowserPersistDocumentChild::AllocPWebBrowserPersistDocumentReadChild() > +{ > + auto* actor = new nsWebBrowserPersistDocumentReadChild(); > + NS_ADDREF(actor); nsRefPtr::forget returns an already_AddRefed, so I'd need actor.forget().take(). But there's also an NS_RELEASE in the Dealloc function, so I'd argue that it's not entirely bad to have an NS_ADDREF balancing it. ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentParent.cpp @@ +44,5 @@ > +nsWebBrowserPersistDocumentParent::FireOnReady() > +{ > + MOZ_ASSERT(!WaitingForAttrs()); > + MOZ_ASSERT(mHoldingExtraRef); > + MOZ_ASSERT(mOnReady); I think this is a valid assertion? The ActorDestroy call site guarantees it directly, and the two others both test that WaitingForAttrs() is true and then do things that make it false before calling this method, so even if the child violates the protocol state machine to send multiple Attributes/InitFailure methods then FireOnReady will still be called only once. But either I'm wrong about that, or I'm right but it's not obvious to a careful reader, and both of these are bad. I think this will be simpler when this class is split into XPCOM and IPC concerns. ::: toolkit/content/contentAreaUtils.js @@ +126,4 @@ > aDoc, aSkipPrompt, null); > } > > +function saveFrame(aFrame, aSkipPrompt) That will look nicer if I also rename this to saveBrowser (with a comment clarifying that any frame-like element will also work), which conveniently avoids confusion with the saveFrame in the context menu code and makes the callers look like saveBrowser(gBrowser.selectedBrowser). @@ +149,2 @@ > > + if ("contentDisposition" in aDocument && "cacheKey" in aDocument) { The idea behind the "in"s was to be more duck-typed, the same way that autoconf style is to test for features rather than a particular OS, but that may not make sense here and it may not make any difference anyway. @@ +173,5 @@ > + .currentDescriptor > + .QueryInterface(Components.interfaces.nsISHEntry); > + > + let cacheKey = shEntry.cacheKey > + .QueryInterface(Components.interfaces This and `var ifreq` were like that when I found them, but it probably can't hurt to clean them up while moving and reindenting. @@ +176,5 @@ > + let cacheKey = shEntry.cacheKey > + .QueryInterface(Components.interfaces > + .nsISupportsPRUint32) > + .data; > + // cacheKey might be a CPOW, but numbers aren't wrapped. That's a good point about add-ons. If any add-ons are doing that for some reason (and it isn't the top-level document of a browser that lives in the parent) then we don't have a good answer yet. Hopefully the fix for the context menu in bug 1141337 would also be usable for other privileged script that wanted to save non-top-level documents. @@ +844,4 @@ > > function getPostData(aDocument) > { > + if ("postData" in aDocument) { I guess this should also be an instanceof, as long as I'm changing all of the others? @@ +955,4 @@ > } > > let docTitle; > + if (aDocument && aDocument.title) { That looks like it was added for an earlier version where the attribute didn't exist, so validateFileName would try to do things to undefined and make getDefaultFileName terminate abnormally by throwing an exception up to initFileInfo, if I'm following all of this correctly. So, yes, this line can be reverted now; thanks.

(Parts of comment #28 will make more sense when viewed in splinter alongside the corresponding parts of comment #25 that they're meant as responses to; I didn't realize they'd be posted with only quotes of the patch when I wrote them.)

Comment on attachment 8634955 [details] [diff] [review] Big Patch (2015-07-16) Review of attachment 8634955 [details] [diff] [review]: ----------------------------------------------------------------- ::: embedding/components/webbrowserpersist/nsWebBrowserPersistDocumentParent.cpp @@ +263,5 @@ > + nsresult rv = AccessAttrs(); > + if (NS_SUCCEEDED(rv)) { > + nsCOMPtr<nsIInputStream> stream = > + mozilla::ipc::DeserializeInputStream(mAttrs->postData(), > + mAttrs->postFiles()); I think this actually is wrong, as the FIXME suggests… but in normal usage postFiles is empty, because the nsIInputStream in question is a [MIME stream wrapped around a Multiplex stream wrapped around a] RemoteInputStream, over in dom/ipc/Blob.cpp, which is serialized as a RemoteInputStream and apparently lazily IPC'ed. (The problem I was alluding to: a receiving-side FileDescriptor asserts that the PlatformHandle method is called at least once, because it doesn't take ownership so not calling it is a leak; also, calling it multiple times in this way is a use-after-close. But there are no actual FileDescriptor objects here.)

(In reply to Jed Davis [:jld] {UTC-7} from comment #30) > apparently lazily IPC'ed. To correct the historical record: no, Blobs are more magical than that; the parent recognizes that it already has a representation of it and eventually reopens the file.

Hey Jed, Out of curiosity, why is it necessary for nsFrameLoader to implement nsIWebBrowserPersistable? Can we not just add a method "persistSubFrame" to nsIFrameLoader that does the actions of nsFrameLoader::StartPersistence, that takes a (maybe optional, if possible) outer window ID? That, to me, would help with bug 1141337, as I'll need to message the outer window ID down to the content process at some point in order to get the right nsIDocument to call StartPersistence on. Or am I missing something?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #33) > Out of curiosity, why is it necessary for nsFrameLoader to implement > nsIWebBrowserPersistable? Can we not just add a method "persistSubFrame" to > nsIFrameLoader that does the actions of nsFrameLoader::StartPersistence, > that takes a (maybe optional, if possible) outer window ID? That's a good point — with the change to make nsIWBP::saveDocument take only a DOMDocument or a WBPDocument and not a Persistable, it looks like the nsDocument implementation of Persistable isn't used. C++ code calls the constructor directly, and JS can't access that XPCOM method because nsDocument is WebIDL. I'd also been trying to keep changes to non-WebBrowserPersist code to a minimum, but I don't know if there'd be any objection to adding a WebBrowserPersist-specific methods to nsIFrameLoader. > That, to me, would help with bug 1141337, as I'll need to message the outer > window ID down to the content process at some point in order to get the > right nsIDocument to call StartPersistence on. I'd been assuming that bug 1141337 would need to add a way for the parent to ask the child about a specific subdocument, and that it shouldn't be too hard to add that after this patch.

With all of the suggested changes from the original review, except still with nsClassName names. Also renames a bunch of the classes to be, hopefully, more understandable. There's a FIXME because I need to double-check the `delete this` rules. Still has nsIWebBrowserPersistable, for now.

Bill: any opinions on the exact shade of bikeshed paint for the class names (comment #27) and/or whether replacing nsIWebBrowserPersistable with a method on nsIFrameLoader is okay (comment #33, comment #34)?

(In reply to Jed Davis [:jld] {UTC-7} from comment #27) > (In reply to Bill McCloskey (:billm) from comment #25) > > It would be nice to get rid of the 'ns' namespacing for at least these new > > files. > > So, mozilla::WebBrowserPersistClassName? Yes, that one. > The XPIDL can't be namespaced, and I've seen the occasional mozIThing but > given that there's an established nsIThing here it seems better to stick > with that for the XPCOM interfaces? Yes, nsIFoo for the interfaces. > replacing nsIWebBrowserPersistable with a method on nsIFrameLoader is okay Either way is fine with me. I haven't thought enough about saving frames to have an opinion about which is better.

Carrying over r=billm. Now properly formatted. nsDocument is no longer changed, and the new classes are namespaced (which allows pruning a bunch of mozilla::, which is nice). Also fixed a trivial static analysis breakage found by Try, and a couple of cosmetic things, and bumped the UUID on nsIWebBrowserPersist for the argument type change.

Jed, I am the developer of the "Print Edit" add-on, which I am currently migrating to e10s. The only feature in Print Edit that does not work in e10s is saving the DOM as "Web Page, Complete". I have tried using the new saveBrowser() function, but it hangs without throwing any exceptions or logging any error messages. The DOM is slightly unusual because it is the static cloned print-preview DOM generated by CreateStaticClone() in nsDocument.cpp, which is called from Init() in nsPrintObject.cpp, which is called from PrintPreview() in nsPrintEngine.cpp. Using saveDocument() to save this print-preview DOM worked fine in earlier versions of Firefox. Do you know any reason why this doesn't work with saveBrowser() in e10s ? Dave

There are two things going on here: 1. The print preview doesn't have a page descriptor, and the WebBrowserPersistLocalDocument getters for cacheKey and postData don't ignore exceptions the way the contentAreaUtils code used to. That's definitely a bug. (There are NS_WARNING messages about that, but those are printed only in debug builds.) 2. saveBrowser() doesn't give an onError callback to startPersistence, because I thought there was nothing useful I could do there. That wasn't quite right — logging anything would help somewhat, and throwing a Components.Exception would even turn the nsresult back into a symbolic name — but the failure was in C++ code in the child process, so it wouldn't point directly to the failure. It's still better than nothing, and easy to fix. I'll go file a bug.

Thanks for doing the investigation and analysis - and so quickly! I was fearing that the issues might be too difficult to fix. The alternative solutions (that I could think of) would all have resulted in reduced or compromised functionality in Print Edit. I will re-test Print Edit in e10s and non-e10s windows when the fix is available.

I have re-tested Print Edit in e10s and non-e10s windows with this fix and everything works correctly.

Reproduced this bug on Firefox Nightly 36.0a1 (2014-11-18) on Linux,64 bit This Bug is now verified as fixed on Latest Firefox beta 42.0b3 (Build ID: 20151001142456) User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:42.0) Gecko/20100101 Firefox/42.0 and Latest Firefox Nightly 44.0a1 (2015-10-01) (Build ID: 20151001030236) User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0

I have reproduced this bug on Firefox nightly 36.0a1 (2014-11-18) on windows(64bit). this bug is verified as fixed . Latest Firefox nightly 44.0a1 (2015-10-01) on windows(64bit) Build Id: 20151001030236 user Agent:Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0. Latest Firefox beta version:42.0a2 (2015-09-11) on windows(64bit) Build Id: 20150911004112 user Agent :Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0

As per Comment 45 and Comment 46, The bug is now verified as fixed ! Marking it as verified!