Per bsmedberg's request, I'm attaching a rather large patch (of all changes in e10s not yet merged to m-c). I've gone through the code and made sure all the TODO/XXX's in the necko e10s code have bugs assigned to them (there are some XXX's in the patch, but they are simply the result of moving some code into HttpBaseChannel: the XXX's are currently in nsHttpChannel in m-c). There are still XXX/TODO's in the non-necko parts of the patch (especially test code): I figure others are better qualified to attach them to existing bugs or create new ones. Assigning sr to bz, since I'm 95% sure that's what bsmedberg wants. I'm sure he want you to look over the necko stuff, bz, but I'm not sure what the plan is for the other bits. Note that there are some files (mainly test JS code) that have Windows ^M carriage returns in them--convert before merge? This patch was generated by running the following in my e10s tree: hg diff -r b8fb5dfc389a:tip AFAICT that appears to be the most recent changeset merged in from M-C. http://hg.mozilla.org/projects/electrolysis/rev/b8fb5dfc389a

Comment on attachment 450288 [details] [diff] [review] diff of e10s and m-c, based off rev b8fb5dfc389a PS. the tip revision when this patch was made was 780d60ef1a75, so you can find further changes to the e10s tree by diffing from that.

There are still DROP_DEADs in the HTTP code that look like they could be hit (esp. if the plugin host is in the content process). Is that expected?

I think jason is going to remove them very quickly. In our e10s patch queue, we have them disabled: http://hg.mozilla.org/users/dougt_mozilla.com/e10s-patches/file/f97863d4dddc/NECKO_WORKAROUND_XXX

Comment so far: >+++ b/chrome/src/nsChromeFactory.cpp >+#ifdef MOZ_IPC >+ if (XRE_GetProcessType() == GeckoProcessType_Content) >+ chromeRegistry = new nsChromeRegistryContent; >+#endif >+ if (!chromeRegistry) >+ chromeRegistry = new nsChromeRegistryChrome; Might be a good idea to make sure we don't end up with an nsChromeRegistryChrome in a content process by tossing an |else| in there? >+++ b/chrome/src/nsChromeRegistry.cpp >+nsChromeRegistry::GetService() >.. >+ if (!nsChromeRegistry::gChromeRegistry) Why the qualifier? >+ NS_IF_ADDREF(gChromeRegistry); Can't be null there. >@@ -693,157 +331,44 @@ nsChromeRegistry::ConvertChromeURL(nsIUR >+ rv = GetBaseURIFromPackage(package, provider, path, &baseURI); That's a really confusing signature. Followup to just return nsIURI? >+++ b/content/base/public/nsIFrameLoader.idl >+ readonly attribute nsIWebProgress webProgress; We should document that this is always available, even in multiprocess situations. We should also document that docShell is NOT always available. >+ * Activate remote frame. >+ * Throws an exception with non-remote frames. >+ */ >+ void activateRemoteFrame(); That needs a lot more documentation. What does "activate" mean, exactly? >+ * @see nsIDOMWindowUtils sendMouseEvent. >+ */ >+ void sendCrossProcessMouseEvent(in AString aType, What happens if this is called on a same-process frame? >+ * Activate event forwarding from client (remote frame) to parent. >+ */ >+ void activateFrameEvent(in AString aType, in boolean capture); Same here. >+ * @see nsIDOMWindowUtils sendKeyEvent. >+ */ >+ void sendCrossProcessKeyEvent(in AString aType, And here. >+ attribute boolean delayRemoteDialogs; This needs documenting. >+ readonly attribute nsIVariant crossProcessObjectWrapper; So does this, even more so. What sorts of things could there be in this variant? Integers? Strings? Something else? >+++ b/content/base/src/nsFrameLoader.cpp >@@ -542,73 +648,87 @@ nsFrameLoader::Show(PRInt32 marginWidth, >+ if (presShell) >+ return true; PR_TRUE, no? There are several places in this file that look like this: if (!mChildProcess) { TryNewProcess(); } if (!mChildProcess) { // return error } Seems like the second if should be inside the first, no? >+ GetMessageManager(getter_AddRefs(dummy)); // Initialize message manager. Just EnsureMessageManager(), maybe? >+nsFrameLoader::GetCrossProcessObjectWrapper(nsIVariant** cpow) Once we can return jsvals directly, perhaps we should do that here? +++ b/content/base/src/nsFrameLoader.h >+ NS_IMETHOD GetCrossProcessObjectWrapper(nsIVariant** cpow); Why not just nsresult? >+++ b/content/canvas/public/DocumentRendererChild.h There are tabs in here (and various other places in this patch). Please fix. And please add modelines ot prevent them from reinfiltrating. >+class DocumentRendererChild : public PDocumentRendererChild Please document what this class is supposed to do (ideally in mxr-form)? And perhaps document the ipdl to describe what the actors represent and what the contract is (specifically that just constructing a PDocumentRenderer from the parent's side will do the rendering and then communicate ther results back via the __delete__)? In fact, decent docs on the ipdl for this and its manager might be good enough. >+ bool RenderDocument(nsIDOMWindow *window, const PRInt32& x, const PRInt32& y, const PRInt32& w, const PRInt32& h, Document what this does (at least by reference to whatever would explain what |flags| means)? What coordinate space and units are the various integers in? This applies to all the renderers, child and parent. >+++ b/content/canvas/public/DocumentRendererNativeIDChild.h Same issues here. >+++ b/content/canvas/public/DocumentRendererNativeIDParent.h And here. >+++ b/content/canvas/public/DocumentRendererParent.h And here. >+++ b/content/canvas/public/DocumentRendererShmemChild.h And here. What's the gfxMatrix arg? >+++ b/content/canvas/public/DocumentRendererShmemParent.h And here. >+++ b/content/canvas/public/nsICanvasRenderingContextInternal.h >+ // Redraw the dirty rectangle of this canvas. >+ NS_IMETHOD Redraw(const gfxRect &dirty) = 0; In what units/coords? >+ // If this context can be set to use Mozilla's Shmem segments as its backing >+ // store, this will set it to that state. Note that if you have drawn >+ // anything into this canvas before changing the shmem state, it will be >+ // lost. "it" == "what you have drawn", not "the shmem state", right? Clarify? >+ // Swap this back buffer with the front, and copy its contents to the new >+ // back. x, y, w, and h specify the area of |back| that is dirty. In which coord system? What happens if you call this on a non-shmem canvas? >+ // Sync back and front buffer, move ownership of back buffer to parent Similar here. Also, what does nativeID mean? >+++ b/content/canvas/src/DocumentRendererChild.cpp >+FlushLayoutForTree(nsIDOMWindow* aWindow) Can we just move this to nsContentUtils instead of copy/pasting it all over? Wereally don't want 4 copies of this method around. >+ _width = nsPresContext::AppUnitsToIntCSSPixels(w); >+ _height = nsPresContext::AppUnitsToIntCSSPixels(h); Really? If the args are app units, shouldn't they be nscoord instead of PRInt32? This seems to apply throughout here. Should there be sanity checks for negative w and h here? >+ nsRefPtr<gfxImageSurface> surf = new gfxImageSurface(reinterpret_cast<PRUint8*>(const_cast<char*>(data.get())), Why is that const_cast ok? That should be a BeginWriting(), since that's what you're doing. The magic "4" scattered about here should presumably be some named constant somewhere. FIle a bug on that? >+++ b/content/canvas/src/DocumentRendererNativeIDChild.cpp >+ XSync(dpy, False); I take it there's no way to avoid that? >+++ b/content/canvas/src/WebGLContext.h >+ NS_IMETHOD Swap(mozilla::ipc::Shmem& aBack, >+ PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h) >+ { return NS_ERROR_NOT_IMPLEMENTED; } >+ NS_IMETHOD Swap(PRUint32 nativeID, >+ PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h) >+ { return NS_ERROR_NOT_IMPLEMENTED; } Callers ignore the retval. Should these be NS_NOTREACHED for good measure? >+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp >+ nsRefPtr<gfxASurface> mBackSurface; >+ // for rendering with NativeID protocol, we should track backbuffer ownership >+ PRPackedBool mIsBackSurfaceReadable; To pack well, these two should be in the opposite order. >+ ContentProcessParent* allocator = ContentProcessParent::GetSingleton(PR_FALSE); >+ if (allocator && gfxSharedImageSurface::IsSharedImage(mBackSurface)) { >+ Shmem mem = static_cast<gfxSharedImageSurface*>(mBackSurface.get())->GetShmem(); >+ allocator->DeallocShmem(mem); >+ } Would be good to not have two copies of that code. Inline function or macro, please? >+ if (mBackSurface && >+ mBackSurface->GetType() == gfxASurface::SurfaceTypeXlib) { Fix indent. More tabs here? >+++ b/content/html/content/src/nsHTMLCanvasElement.cpp >+ ctxId.Assign(NS_LossyConvertUTF16toASCII(aContextId)); LossyCopyUTF16toASCII(aContextId, ctxId); >+++ b/content/xul/content/src/nsXULElement.cpp >+nsXULElement::GetCrossProcessObjectWrapper(nsIVariant** cpow) >+{ >+ nsRefPtr<nsFrameLoader> frameLoader(GetFrameLoader()); >+ return frameLoader->GetCrossProcessObjectWrapper(cpow); GetFrameLoader() can return null. >diff --git a/dom/base/Makefile.in b/dom/base/Makefile.in >+ifdef MOZ_IPC >+include $(topsrcdir)/config/config.mk >+include $(topsrcdir)/ipc/chromium/chromium-config.mk >+endif The other includesof chromium-config.mk didn't ifdef MOZ_IPC it. Which is the right way? > include $(topsrcdir)/config/rules.mk Why not just move this to before the ifdef above? >+++ b/dom/interfaces/html/nsIDOMHTMLCanvasElement.idl >+ // A Mozilla-only extension to get a canvas context backed by double-buffered I really wish we had IDL magic to hide this from content script. :( >+++ b/dom/ipc/ContentProcessProcess.h >+/** >+ * ContentProcessProcess is a singleton on the content process which represents >+ * the main thread where tab instances live. >+ */ Put this right after the license block so it shows up in mxr? >+++ b/dom/ipc/PContentDialog.ipdl >+ __delete__(PRInt32[] aIntParams, nsString[] aStringParams); You know, the fact that the delete lives on the protocol while the create lives on the manager is a bit of a pain in terms of figuring out what the protocol would do... Please document as a substitute? >+++ b/dom/ipc/PContentProcess.ipdl Please document what this protocol represents? And what the various messages mean? What do the rv values for all the pref stuff mean, for example? >+++ b/dom/ipc/PDocumentRenderer.ipdl Document. >+++ b/dom/ipc/PDocumentRendererNativeID.ipdl Document. >+++ b/dom/ipc/PDocumentRendererShmem.ipdl Document. >+++ b/dom/ipc/PIFrameEmbedding.ipdl Document. >+++ b/dom/ipc/TabChild.cpp There are a lot of NS_ERROR_NOT_IMPLEMENTED methods here. Are they expected to not be called? Should they have NS_NOTREACHED in them? >+TabChild::ParamsToArrays(nsIDialogParamBlock* aParams, >+ PRUnichar* str = nsnull; >+ while (NS_SUCCEEDED(aParams->GetString(j, &str))) { >+ nsAdoptingString strVal(str); Why not just make strVal an nsXPIDLString and use getter_Copies and skip the |str| temporary? >+++ b/dom/src/geolocation/nsGeolocation.cpp >+nsGeolocation::RegisterRequestWithPrompt(nsGeolocationRequest* request) >+ nsCOMPtr<nsITabChild> tabchild = do_GetInterface(owner); Shouldn't that be QI? >+++ b/embedding/base/nsIDialogCreator.idl This needs documentation. >+++ b/js/ipc/PObjectWrapper.ipdl I'm not sure I understand the ownership model of PObjectWrapper. Worth documenting.

Comment on attachment 450288 [details] [diff] [review] diff of e10s and m-c, based off rev b8fb5dfc389a >+++ b/netwerk/cookie/PCookieService.ipdl >+ sync GetCookieString(URI host, >+ URI originating, >+ bool fromHttp) >+ returns (nsCString result); What does "URI host" mean? Is the caller expected to just send over some URI with the right hostname, or do they need to trim everything except scheme+host(+port?). Or something else? What's |originating|? >+ SetCookieString(URI host, >+ URI originating, >+ nsCString cookieString, >+ nsCString serverTime, >+ bool fromHttp); Similar here. Also, document what serverTime's format is (or at least where it should come from)? >+++ b/netwerk/cookie/nsCookieService.cpp We seem to be adding a lot of code duplicate to this file. Why can't we push the GetOriginatingURI stuff and whatnot into the *Internal methods? >+ // Not passing an nsIChannel here means CanSetCookie will use the currently >+ // active window to display the prompt. This isn't exactly ideal... Please file a followup bug and cite the bug# here. >+++ b/netwerk/ipc/NeckoMessageUtils.h >+ // The contained URI is already addrefed on creation. We don't want another >+ // addref when passing it off to its actual owner. >+ operator nsCOMPtr<nsIURI>() const { return already_AddRefed<nsIURI>(mURI); } This is leak-prone. For example, just in this patch we leak URIs if CookieServiceParent::RecvGetCookieString happens when !mCookieService. Not only that, but f you make the mistake of assigning to two different nsCOMPtrs you end up double-releasing. Are we really that worried about the performance impact of refcounting the URIs when passing them across the ipdl boundary? As implemented right now we have no addref/release on the content side, but on the chrome side we have: 1) An addref when creating. 2) An addref/release pair when assigning to an nsCOMPtr 3) A release when that nsCOMPtr goes out of scope. If we made the member an nsCOMPtr and just had the operator() return nsIURI*, we would have an addref/release on the content side, but no change in number of addrefs/releases on the chrome side. If we made operator() return already_AddRefed<nsIURI> (by returning mURI.forget()), we would eliminate one addref/release pair on the chrome side. In either case, Read() would swap() into mURI. I'd prefer one or the other of the above solutions to what we have now. >+ URI& operator=(URI&); This is purposefully unimplemented, right? Should that be documented? Should the copy ctor similarly be private and unimplemented? >+++ b/netwerk/ipc/PNecko.ipdl >+++ b/netwerk/protocol/http/PHttpChannel.ipdl >+ AsyncOpen(URI uri, >+ // - TODO: bug 571161: unclear if any HTTP channel clients ever >+ // set originalURI != uri (about:credits?); also not clear if >+ // chrome channel would ever need to know. Get rid of next arg? >+ URI original, >+ URI doc, Probably worth documenting at least the |doc| argument. >+ PRInt32 uploadStreamInfo, Document? That's pretty opaque. >+ OnStartRequest(nsHttpResponseHead responseHead); >+ >+ OnDataAvailable(nsCString data, >+ PRUint32 offset, >+ PRUint32 count); >+ >+ OnStopRequest(nsresult statusCode); Document that these follow the usual nsIChannel contract (if they do) or what contract they follow otherwise? sr=bzbarsky. I don't think any of the above should block this landing (except maybe the IPC::URI leaks if they might get hit); please file followup bugs as needed.

Oh, I completely skipped over the test gunk at the end and the majority of the HTTP .cpp changes, which seem to be code relocation.

Comment on attachment 450288 [details] [diff] [review] diff of e10s and m-c, based off rev b8fb5dfc389a >+++ b/.hgtags >+941ad9d7d079246481f365c3cfbfc75a5bbefc94 last-mozilla-central >+2bae3bbf866e7de2a4b2377e7c2f52cc9ac14a22 last-mozilla-central >+2bae3bbf866e7de2a4b2377e7c2f52cc9ac14a22 last-mozilla-central >+65c1582465efe99899189519fccaf7b2826fcb2e last-mozilla-central >+65c1582465efe99899189519fccaf7b2826fcb2e last-mozilla-central >+27937722da69ad0e8fd140a00671413068226a5b last-mozilla-central >+27937722da69ad0e8fd140a00671413068226a5b last-mozilla-central >+a732c6d3c078f80635255c78bfaadffa5828a8a5 last-mozilla-central >+a732c6d3c078f80635255c78bfaadffa5828a8a5 last-mozilla-central >+925595f3c08634cc42e33158ea6858bb55623ef7 last-mozilla-central >+dba2abb7db57078c5a4810884834d3056a5d56c2 last-mozilla-central Nit on my own part: can we keep these out of m-c or alternatively just apply a null-rev tag of last-mozilla-central. (to effectively delete the tag)

this is first reviewed patch, partially applied on m-c b51803f3fdef, no rejects fixed.

Probably some parts of this patch was reviewed already in first patch... but not so many... review?

Oh this part need to be ignored from second patch (accidentally pushed fix for bug 574581 into e10s branch.) ThebesLayerOGL.cpp ThebesLayerOGL::ThebesLayerOGL , mOffscreenFormat(gfxASurface::ImageFormatUnknown) + , mOffscreenSize(-1,-1)

Removed wrong change from patch

Which part of that last patch do you actually need reviewed? Certainly the chrome reg changes look like stuff I've seen already....

(In reply to comment #12) > Which part of that last patch do you actually need reviewed? Certainly the > chrome reg changes look like stuff I've seen already.... yes, because it was changed repeatedly after revision which you have reviewed already

e10s changesets: 103b167938e5 1727bfc11147 20a3f6401423 250fd088e49d 2dff2dec513f 2f5993d0b7e8 3825724e7f06 5ec45d0b2b67 712c3d00ba30 780d60ef1a75 7f7b9ee37084 8e4ceb3b4da5 b480c5f26a70 c11f32e403db c62406f6a060 deaa565e6e85 e3bf810e9159 e9367578b98a ebeb7279db4a f0474518bd43 f2ef80cc432f f5adde791b93 f80302dc4c6c

Comment on attachment 454575 [details] [diff] [review] pushlog changsets Just hide this patch

OK, after going through a list of the patches that have landed since b8fb5dfc389a: http://hg.mozilla.org/projects/electrolysis/pushloghtml/1 I only see one that I think really needs +sr that doesn't already have it. It's bug 537782 (HTTP Auth), and fairly small (it's based on a larger patch, for bug 569227) that smaug already +sr'd).

I think we should file followup bugs to fix the issues bz has identified, and close this bug once we have the wave-through for bug 537782 (apparently stuart looked at it?) dwitte: you're the right person to address the cookie/URI issues in comment 5, I assume? Doug: I'm hoping you can address the stuff in comment 4, or know who to put on it.

all of the stuff in comment 4? bsmedberg, who owns this clean up?

> It's bug 537782 (HTTP Auth) I just commented on that.

I filed meta bug 575636 to address the concerns raise by Bz in comment 4.

http://hg.mozilla.org/mozilla-central/rev/3aff97777291 Thanks all.

Fwiw, these were my merge commands. Both e10s and m-c were up to date and had no local changes. i had two local clones. one for m-c, one for e10s. 581 cd mozilla-central/ 584 hg pull ../electrolysis 585 hg merge 5149a5980945 5149a5980945 was the changeset tip of the e10s branch 586 hg commit -m"Bug 571166 - merge from e10s to m-c. r=various. sr=bz. CLOSED TREE." 587 hg push ssh://hg.mozilla.org/mozilla-central

bz: re: comment 5, when you say we should document whether the IPDL OnStartRequest/OnData/Stop methods follow the usual nsIChannel contract, you simply mean whether OnStart/Stop are guaranteed to be called once AsyncOpen has been called? Anything else?

Pretty much that, yes. And whether OnData must consume all data in the stream.