We want the b2g browser to be separate from the main gecko process. We also want a bunch of properties for <iframe mozbrowser> (history isolation, window.top isolation, etc.) as tracked in bug 693515. We'll also need most of these properties for b2g apps. It seems like the simplest thing which could possibly work is to always run <iframe mozbrowser> in a content process. That should get us most of bug 693515 for free, aiui. If it later turns out that we really want <iframe mozbrowser> without content processes, then we can make that work. But since we're pretty sure we want an out-of-process browser, I think we should do that first. cjones, I'm happy to do this, but I don't know where to start.

How is this different from <xul:iframe> or the core bits of <xul:browser>? Can we just use those with remote="true"?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > How is this different from <xul:iframe> or the core bits of <xul:browser>? It's basically the same, except that the parent is a content HTML window, not a chrome XUL window. > Can we just use those with remote="true"? Maybe?

We want to extend the security checks here http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1768 and a case where we look for "mozbrowser". Everything should magically work after that (except that it won't!). If/when it doesn't, we need to file bugs for implementing in gecko what the fennec frontend currently does.

Although considering that touch events don't work well with the current browser, maybe everything will Just Work to the current level of functionality.

I guess what I don't understand is why you can't just use xul:iframe type="content", or what differences in behavior you want compared with xul:iframe type="content". Apart from our restriction that you can't use XUL elements from web content (which we could lift for this specific case), it seems easier to reuse the existing code than invent a different element type.

We need to add a lot of new APIs to support the new "browser"-type iframe so there's other motivation for adding it, and we want to standardize those APIs. I'm happy to reuse as much existing code in gecko as we can, but we can't build on XUL if we want to standardize.

I'm not sure how this works with <xul:iframe type="content">, but we have the following problem with <iframe mozbrowser>: EnsureFrameLoader is called from BindToTree. But nsIContent::SetPrimaryFrame is not called until after BindToTree completes. A remote iframe needs its nsIFrame set (via SetPrimaryFrame) before we can create its TabParent. So this means that we can't create the TabParent until after SetPrimaryFrame runs, so in particular, we can't create the TabParent during BindToTree. Is there some event I can bind to for the purposes of calling EnsureFrameLoader later in this cycle? nsGenericHTMLFrameElement::EnsureFrameLoader nsGenericHTMLFrameElement::LoadSrc nsGenericHTMLFrameElement::BindToTree nsINode::doInsertChildAt nsGenericElement::InsertChildAt nsINode::ReplaceOrInsertBefore nsINode::ReplaceOrInsertBefore nsINode::InsertBefore nsINode::AppendChild nsIContent::SetPrimaryFrame nsSubDocumentFrame::Init nsCSSFrameConstructor::InitAndRestoreFrame nsCSSFrameConstructor::ConstructFrameFromItemInternal nsCSSFrameConstructor::ConstructFramesFromItem nsCSSFrameConstructor::ConstructFramesFromItemList nsCSSFrameConstructor::ContentAppended PresShell::ContentAppended nsNodeUtils::ContentAppended nsINode::doInsertChildAt nsGenericElement::InsertChildAt nsINode::ReplaceOrInsertBefore nsINode::ReplaceOrInsertBefore nsINode::InsertBefore nsINode::AppendChild

> Is there some event I can bind to for the purposes of calling EnsureFrameLoader later in this cycle? I guess I can try calling EnsureFrameLoader() asynchronously.

> I guess I can try calling EnsureFrameLoader() asynchronously. This doesn't work even without forcing a child process, but I'm not sure why. The iframe is blank, despite a successful LoadURI('http://mozilla.org') call. I'll dig in tomorrow, but if anyone knows what's going on off the top of their head, I'd certainly appreciate a pointer. I've attached my (tiny) patch.

Are you seeing the page load attempted in the content process? On what OS are you testing?

Comment 9 is explicitly without content processes. In-process iframe, just asynchronously creating the frame loader. I'm on Linux-64.

Ah OK, I didn't parse your first sentence correctly. This is beyond my ken, sorry :(. bz, any ideas?

I thought we generally did frame loader init off a script runner nowadays... In any case, I'd start by trying to use a script runner for your LoadSrc call.

Yay, that worked. Thanks, bz.

Well...the script runner works for the non-cross-process case. But the script runner runs before SetPrimaryFrame runs.

The script runner can run before SetPrimaryFrame runs, yes. Why is that a problem? We should be able to create frame loaders even when there is no nsIFrame, no? And they need to survive destruction of the nsIFrame....

Looks like just to get the DPI of the screen? I'm not sure why I didn't read more than one line of context before now. void TabParent::SetOwnerElement(nsIDOMElement* aElement) { mFrameElement = aElement; // Cache the DPI of the screen, since we may lose the element/widget later if (aElement) { nsCOMPtr<nsIWidget> widget = GetWidget(); NS_ABORT_IF_FALSE(widget, "Non-null OwnerElement must provide a widget!"); mDPI = widget->GetDPI(); } } ###!!! ABORT: Non-null OwnerElement must provide a widget!: 'widget', file ../../../src/dom/ipc/TabParent.cpp, line 111 mozilla::dom::TabParent::SetOwnerElement(nsIDOMElement*) (/home/jlebar/code/moz/ff-git/debug/dom/ipc/../../../src/dom/ipc/TabParent.cpp:112) nsFrameLoader::TryRemoteBrowser() (/home/jlebar/code/moz/ff-git/debug/content/base/src/../../../../src/content/base/src/nsFrameLoader.cpp:1847) nsFrameLoader::ReallyStartLoadingInternal() (/home/jlebar/code/moz/ff-git/debug/content/base/src/../../../../src/content/base/src/nsFrameLoader.cpp:456) nsFrameLoader::ReallyStartLoading() (/home/jlebar/code/moz/ff-git/debug/content/base/src/../../../../src/content/base/src/nsFrameLoader.cpp:434) nsDocument::MaybeInitializeFinalizeFrameLoaders() (/home/jlebar/code/moz/ff-git/debug/content/base/src/../../../../src/content/base/src/nsDocument.cpp:5470) nsDocument::EndUpdate(unsigned int) (/home/jlebar/code/moz/ff-git/debug/content/base/src/../../../../src/content/base/src/nsDocument.cpp:3999) nsHTMLDocument::EndUpdate(unsigned int) (/home/jlebar/code/moz/ff-git/debug/content/html/document/src/../../../../../src/content/html/document/src/nsHTMLDocument.cpp:2295) ~mozAutoDocUpdate (/home/jlebar/code/moz/ff-git/debug/content/xul/templates/src/../../../../../src/content/xul/templates/src/../../../base/src/mozAutoDocUpdate.h:67) nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*) (/home/jlebar/code/moz/ff-git/debug/content/base/src/../../../../src/content/base/src/nsGenericElement.cpp:4041) nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, unsigned int*) (/home/jlebar/code/moz/ff-git/debug/js/xpconnect/src/../../../dist/include/nsINode.h:1344) nsINode::InsertBefore(nsINode*, nsINode*, unsigned int*) (/home/jlebar/code/moz/ff-git/debug/js/xpconnect/src/../../../dist/include/nsINode.h:473) nsINode::AppendChild(nsINode*, unsigned int*) (/home/jlebar/code/moz/ff-git/debug/js/xpconnect/src/../../../dist/include/nsINode.h:483) nsIDOMNode_AppendChild (/home/jlebar/code/moz/ff-git/debug/js/xpconnect/src/dom_quickstubs.cpp:5480) js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jscntxtinlines.h:311) js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jsinterp.cpp:519) js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jsinterp.cpp:2801) js::RunScript(JSContext*, JSScript*, js::StackFrame*) (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jsinterp.cpp:475) js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jsinterp.cpp:537) js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jsinterp.h:157) js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jsinterp.cpp:569) JS_CallFunctionValue (/home/jlebar/code/moz/ff-git/debug/js/src/../../../src/js/src/jsapi.cpp:5459) nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/jlebar/code/moz/ff-inbound/debug/js/xpconnect/src/../../../../src/js/xpconnect/src/XPCWrappedJSClass.cpp:1510) nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (/home/jlebar/code/moz/ff-inbound/debug/js/xpconnect/src/../../../../src/js/xpconnect/src/XPCWrappedJS.cpp:617) PrepareAndDispatch (/home/jlebar/code/moz/ff-inbound/debug/xpcom/reflect/xptcall/src/md/unix/../../../../../../../src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153) SharedStub (xptcstubs_x86_64_linux.cpp:0)

If we ignore the DPI business, we get a bit further, but apparently don't have a docshell: creating 1! loading http://mozilla.org/, 1 WARNING: NS_ENSURE_TRUE(mDocShell) failed: file ../../../../src/embedding/browser/webBrowser/nsWebBrowser.cpp, line 662 WARNING: mWebNav->LoadURI failed. Eating exception, what else can I do?: file ../../../src/dom/ipc/TabChild.cpp, line 512 [TabChild] SHOW (w,h)= (0, 0) __restore_rt (sigaction.c:0) mozilla::layout::RenderFrameParent::AllocPLayers(mozilla::layers::LayerManager::LayersBackend*) (/home/jlebar/code/moz/ff-git/debug/layout/ipc/../../../src/layout/ipc/RenderFrameParent.cpp:645) mozilla::layout::PRenderFrameParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PRenderFrameParent.cpp:247) mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PContentParent.cpp:1771) mozilla::ipc::SyncChannel::OnDispatchMessage(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/SyncChannel.cpp:175) mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/RPCChannel.cpp:432) void DispatchToMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)()>(mozilla::ipc::RPCChannel*, bool (mozilla::ipc::RPCChannel::*)(), Tuple0 const&) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/chromium/src/base/tuple.h:384) RunnableMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)(), Tuple0>::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/chromium/src/base/task.h:308) mozilla::ipc::RPCChannel::RefCountedTask::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../dist/include/mozilla/ipc/RPCChannel.h:462) mozilla::ipc::RPCChannel::DequeueTask::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../dist/include/mozilla/ipc/RPCChannel.h:485) MessageLoop::RunTask(Task*) (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:319) MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:329) MessageLoop::DoWork() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:426) mozilla::ipc::DoWorkRunnable::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:71) nsThread::ProcessNextEvent(bool, bool*) (/home/jlebar/code/moz/ff-inbound/debug/xpcom/threads/../../../src/xpcom/threads/nsThread.cpp:657) NS_ProcessNextEvent_P(nsIThread*, bool) (/home/jlebar/code/moz/ff-inbound/debug/xpcom/build/nsThreadUtils.cpp:245) mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:107) MessageLoop::RunInternal() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:209) MessageLoop::RunHandler() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:202) MessageLoop::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:174) nsBaseAppShell::Run() (/home/jlebar/code/moz/ff-git/debug/widget/xpwidgets/../../../src/widget/xpwidgets/nsBaseAppShell.cpp:191) nsAppStartup::Run() (/home/jlebar/code/moz/ff-inbound/debug/toolkit/components/startup/../../../../src/toolkit/components/startup/nsAppStartup.cpp:220) XRE_main (/home/jlebar/code/moz/ff-git2/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3537) do_main (/home/jlebar/code/moz/ff-git2/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:205) main (/home/jlebar/code/moz/ff-git2/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:295) __libc_start_main (/build/buildd/eglibc-2.13/csu/libc-start.c:258) _start (dist/bin/firefox)

Um. Did e10s never get things working with display:none iframes? I think you should ignore the SetPrimaryFrame issue; it's a red herring. Make <iframe mozbrowser style="display: none" work correctly. Then you get the rest for free. Why is nsWebBrowser not initializing its docshell in the display:none scenario?

Well, it looks like TabChild does not nsIBaseWindow::Create its nsWebBrowser until TabChild::RecvShow(). And there's no docshell in the nsWebBrowser until it's Create()'ed.

Ho-hum. That's kinda broken.... Can we Create earlier?

We wait until RecvShow(), because BaseWindow::Create needs a fake widget, and we can't hook that up properly until we find a real widget in the parent process. Or at least, that's why things are the way they are now. It's just code, we can rejigger it.

Playing whack-a-null here. If we initialize the window (i.e., create(), etc.) at the beginning of TabChild::RecvLoadURL, we crash at creating 1! [TabChild] EnsureWindowInitialized aSize=(0,0) Program dist/bin/firefox (pid = 11138) received signal 11. Stack: __restore_rt (sigaction.c:0) mozilla::layout::RenderFrameParent::AllocPLayers(mozilla::layers::LayerManager::LayersBackend*) (/home/jlebar/code/moz/ff-git/debug/layout/ipc/../../../src/layout/ipc/RenderFrameParent.cpp:645) mozilla::layout::PRenderFrameParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PRenderFrameParent.cpp:247) mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PContentParent.cpp:1771) mozilla::ipc::SyncChannel::OnDispatchMessage(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/SyncChannel.cpp:175) mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/RPCChannel.cpp:432) void DispatchToMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)()>(mozilla::ipc::RPCChannel*, bool (mozilla::ipc::RPCChannel::*)(), Tuple0 const&) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/chromium/src/base/tuple.h:384) RunnableMethod<mozilla::ipc::RPCChannel, bool (mozilla::ipc::RPCChannel::*)(), Tuple0>::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/chromium/src/base/task.h:308) mozilla::ipc::RPCChannel::RefCountedTask::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../dist/include/mozilla/ipc/RPCChannel.h:462) mozilla::ipc::RPCChannel::DequeueTask::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../dist/include/mozilla/ipc/RPCChannel.h:485) MessageLoop::RunTask(Task*) (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:319) MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:329) MessageLoop::DoWork() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:426) mozilla::ipc::DoWorkRunnable::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:71) nsThread::ProcessNextEvent(bool, bool*) (/home/jlebar/code/moz/ff-inbound/debug/xpcom/threads/../../../src/xpcom/threads/nsThread.cpp:657) NS_ProcessNextEvent_P(nsIThread*, bool) (/home/jlebar/code/moz/ff-inbound/debug/xpcom/build/nsThreadUtils.cpp:245) mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:107) MessageLoop::RunInternal() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:209) MessageLoop::RunHandler() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:202) MessageLoop::Run() (/home/jlebar/code/moz/ff-git/debug/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:174) nsBaseAppShell::Run() (/home/jlebar/code/moz/ff-git/debug/widget/xpwidgets/../../../src/widget/xpwidgets/nsBaseAppShell.cpp:191) nsAppStartup::Run() (/home/jlebar/code/moz/ff-inbound/debug/toolkit/components/startup/../../../../src/toolkit/components/startup/nsAppStartup.cpp:220) XRE_main (/home/jlebar/code/moz/ff-git2/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3537) do_main (/home/jlebar/code/moz/ff-git2/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:205) main (/home/jlebar/code/moz/ff-git2/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:295) __libc_start_main (/build/buildd/eglibc-2.13/csu/libc-start.c:258) _start (dist/bin/firefox) which is probably the problem Chris was referring to in comment 23.

This code wasn't designed to handle arbitrary display:none/default/none/etc transitions. We'll need to refactor for the long run. for now, whack/hack whatever you need to get the basic case up. I assume that's what you're doing

> for now, whack/hack whatever you need to get the basic case up. I assume that's what > you're doing Indeed! When the content process starts up, I get a bunch of failed NS_DispatchToMainThread's from MessagePump::ScheduleWork (20 failures or so) because the main thread pointer is null. It kind of looks like we could be missing messages in the child process as a result, but I'm new to all this code. Are these failures expected?

Actually, those null main thread warnings look benign. All the messages seem to be delivered after the deluge of errors ends.

No, that's not at all expected.

Crashes on browser exit, scrollbars don't work, complete hack. However, it loads a page!

Hm, it doesn't work on Linux due to an apparent race (I got it to work once in the debugger when I set breakpoints in one of the processes). I've been and will continue digging.

This kind of works on Linux. The cross-process content shows up, but not the first time. Once the content appears, you can scroll and interact with the content.

This WIP is a serious hack, in case that's not clear... In about:config, search for "mozbrowser". There should be two prefs. Enable mozbrowser and add this attachment's origin (e.g. "https://bug714861.bugzilla.mozilla.org") to the whitelist pref. This attachment should work on Mac, but it doesn't load the page on Linux. If you wait a few seconds and then refresh the page, the inner page loads successfully on Linux. The same thing happens if you set the iframe's source on the outer page's load, or on the outer page's load plus one trip through setTimeout(0) -- that's too soon.

Restart the browser, then open this testcase. If you wait a second or two after loading the page and then press "go", it'll work on both Mac and Linux.

What appears to be happening is that the IO thread comes up and starts processing messages before nsThreadManager has been initialized (maybe all of XPCOM). But without an XPCOM main thread to post them to, they're being dropped.

Alright, that's not the problem ... I'm testing in a --enable-application=b2g build. What's *actually* happening is that the browser app itself gets loaded out of process, and then it tries to create another child content process for web content. Something in the browser app's content process is apparently unhappy with that. Something we need to fix, but not for this bug.

And FTR, the nested content-process creation is failing because in nsCOMPtr<nsIXULWindow> window(do_GetInterface(parentOwner)); if (!window) { return false; } (gdb) p parentOwner.mRawPtr $2 = (nsDocShellTreeOwner *) 0xb29bd820 so apparently nsDocShellTreeOwner can't find an nsIXULWindow, not too surprising in a content process. I'd like to get this landed but pref'd off asap.

Comment on attachment 613104 [details] [diff] [review] WIP v3 (rebased to tip) Lots of nits in here to pick, but the only thing that makes me particularly unhappy with the new |mForceChildProcess| logic in nsFrameLoader. Why was that needed? We should centralize those checks. Reserving judgment pending answer to that question ;).

The key problem with this patch is that testcase 1 does not work on Linux. Do you have any insight into that?

I didn't test a desktop FF build, so nothing to add. There are only a finite number of things it could be. The question is whether pursuing them is a good use of time now, since many of those causes are linux-x11 or firefox-mouse-interface specific. This is mostly working on desktop-b2g except for IME, but that's not the fault of this patch. About to test in phone-b2g. The right way forward is to clean this up and land it pref'd off, so followup work can go on in parallel. There's a bit ... ;)

I'm concerned that this patch is broken in a fundamental way, which is causing it to work only on the second load issued to a mozbrowser, at least on Linux. Perhaps you don't observe this on desktop b2g because the first load into a mozbrowser is overwritten by a second load. I am not comfortable landing code in gecko -- pref'ed off or not -- which has flaws we do not understand, particularly because there's a good chance that the flaws I've seen on desktop Linux apply also to desktop b2g. I'd really appreciate some assistance from the domain expert in understanding this issue better.

> I am not comfortable landing code in gecko -- pref'ed off or not -- which has flaws we do not > understand, particularly because there's a good chance that the flaws I've seen on desktop Linux > apply also to desktop b2g. I just tested on desktop b2g, and amazingly enough, testcase 1 works there. (I hacked <iframe mozbrowser src="http://mozilla.org"> into the Gaia homescreen.) If things similarly work on the phone, then I'm OK taking this.

Yup, it can load pages on the phone. I'll whip this patch up into shape.

<browser remote> has never been a supported or even tested configuration in desktop-linux-x11/Firefox-mouse. Let alone <iframe mozbrowser>. While I agree with you in the abstract, we need to stay on target, and hop carefully from known-to-work lily pad to the next one we want to work, or we might end up falling into a deep and foul pond. Or something like that.

(In reply to Justin Lebar [:jlebar] from comment #43) > Yup, it can load pages on the phone. I think this is actually untrue, unfortunately. My upload failed and I didn't notice. :( > While I agree with you in the abstract, we need to stay on target Agreed, but it's also important that things work the same on desktop b2g and mobile b2g, since so many people use desktop b2g for development. My assumption was that desktop b2g would behave very much like desktop Firefox, but it appears that was wrong in this case!

This is a patch to Gaia demonstrating the bug in this patch. On my SGSII, I get a blank iframe with mozbrowser. On desktop b2g, I get some content in the iframe. I don't think <iframe src="http://foo"> is an outlandish thing to want to work before we check this in, even pref'ed off. Also, it seems that I can't unlock the homescreen with OOP mozbrowser applied. But maybe something else is going on; I'm getting tired. If you want to check this in ASAP, perhaps we should check it into a git branch. That way, it goes in immediately, warts and all, and others can hack on it. What do you think?

Also, there's a crash on shutdown, presumably in the child process: __restore_rt (sigaction.c:0) nsOfflineCacheDevice::RunSimpleQuery(mozIStorageStatement*, unsigned int, unsigned int*, char***) (/home/jlebar/code/moz/ff-git/debug-b2g/netwerk/cache/../../../src/netwerk/cache/nsDiskCacheDeviceSQL.cpp:2085) nsOfflineCacheDevice::GetGroupsTimeOrdered(unsigned int*, char***) (/home/jlebar/code/moz/ff-git/debug-b2g/netwerk/cache/../../../src/netwerk/cache/nsDiskCacheDeviceSQL.cpp:2076) nsOfflineCacheUpdate::EvictOneNonPinned() (/home/jlebar/code/moz/ff-git/debug/uriloader/prefetch/../../../src/uriloader/prefetch/nsOfflineCacheUpdate.cpp:1937) nsOfflineCacheUpdate::LoadCompleted() (/home/jlebar/code/moz/ff-git/debug/uriloader/prefetch/../../../src/uriloader/prefetch/nsOfflineCacheUpdate.cpp:1450) nsOfflineCacheUpdateItem::Run() (/home/jlebar/code/moz/ff-git/debug/uriloader/prefetch/../../../src/uriloader/prefetch/nsOfflineCacheUpdate.cpp:473) nsThread::ProcessNextEvent(bool, bool*) (/home/jlebar/code/moz/ff-git/debug/xpcom/threads/../../../src/xpcom/threads/nsThread.cpp:656) NS_ProcessNextEvent_P(nsIThread*, bool) (/home/jlebar/code/moz/ff-git/debug/xpcom/build/nsThreadUtils.cpp:245) Spin (/home/jlebar/code/moz/ff-git/debug-b2g/toolkit/components/places/../../../../src/toolkit/components/places/Database.cpp:291) mozilla::places::Database::Shutdown() (/home/jlebar/code/moz/ff-git/debug-b2g/toolkit/components/places/../../../../src/toolkit/components/places/Database.cpp:1863) mozilla::places::Database::Observe(nsISupports*, char const*, unsigned short const*) (/home/jlebar/code/moz/ff-git/debug-b2g/toolkit/components/places/../../../../src/toolkit/components/places/Database.cpp:1956) nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*) (/home/jlebar/code/moz/ff-git/debug/xpcom/ds/../../../src/xpcom/ds/nsObserverList.cpp:129) nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) (/home/jlebar/code/moz/ff-git/debug/xpcom/ds/../../../src/xpcom/ds/nsObserverService.cpp:185) nsXREDirProvider::DoShutdown() (/home/jlebar/code/moz/ff-git/debug-b2g/toolkit/xre/../../../src/toolkit/xre/nsXREDirProvider.cpp:841) ~ScopedXPCOMStartup (/home/jlebar/code/moz/ff-git/debug-b2g/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:1127) XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/jlebar/code/moz/ff-git/debug-b2g/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3871) XRE_main (/home/jlebar/code/moz/ff-git/debug-b2g/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3925)

(In reply to Justin Lebar [:jlebar] from comment #45) > (In reply to Justin Lebar [:jlebar] from comment #43) > > Yup, it can load pages on the phone. > > I think this is actually untrue, unfortunately. My upload failed and I > didn't notice. :( > I'm not against checking this in if it can't load content on the phone, but not being able to load content means there's not much motivation to landitlanditlandit. I had some issues getting a gecko on device to test this patch, so I'll retry this afternoon and poke a little. Likely something dumb. > > While I agree with you in the abstract, we need to stay on target > > Agreed, but it's also important that things work the same on desktop b2g and > mobile b2g, since so many people use desktop b2g for development. My > assumption was that desktop b2g would behave very much like desktop Firefox, > but it appears that was wrong in this case! There are many many many things that are different between FF-mouse frontend and b2g. I don't think that's a good thing, but it's the way it is. Many of those differences affect cross-process compatibility.

I can reproduce the page-not-loading. |ps| says that we're spawning the plugin-container subprocess, but debuggerd shows up not long afterward. Something may be hanging on startup. I'm not able to repro problems with the lockscreen. Let me poke at this a little bit.

Crashes below in the child process. Progress! Now we get to start undoing hackity hacks ... (gdb) bt #0 nsScreenGonk::GetPixelDepth (this=<value optimized out>, aPixelDepth=0xbec229b4) at /home/cjones/mozilla/b2g/glue/gonk-ics/frameworks/base/include/ui/FramebufferNativeWindow.h:55 #1 0x407606e6 in nsPrincipal::SubsumesIgnoringDomain (this=0x0, aOther=0xbec229b4, aResult=0x410dd1f0) at /home/cjones/mozilla/mozilla-central/caps/src/nsPrincipal.cpp:379 #2 0x40b36af6 in gfxAndroidPlatform::gfxAndroidPlatform (this=0x41c361c0) at /home/cjones/mozilla/mozilla-central/gfx/thebes/gfxAndroidPlatform.cpp:67 #3 0x40b2f8a8 in gfxPlatform::Init () at /home/cjones/mozilla/mozilla-central/gfx/thebes/gfxPlatform.cpp:299 #4 0x40b2fac8 in gfxPlatform::GetPlatform () at /home/cjones/mozilla/mozilla-central/gfx/thebes/gfxPlatform.cpp:255 #5 0x409e1238 in mozilla::widget::PuppetWidget::Create (this=0x41ceb440, aParent=0x0, aNativeParent=<value optimized out>, aRect=<value optimized out>, aHandleEventFunction=0, aContext=0x0, aInitData=0x0) at /home/cjones/mozilla/mozilla-central/widget/xpwidgets/PuppetWidget.cpp:127

Since this crash has nothing to do with the patch here and should be fixed in parallel, and we're loading OOP content successfully in desktop-b2g, then please continue on the track of landing what's here pref'd off. We can start filing followups for bugs like these.

Comment on attachment 613666 [details] [diff] [review] Part 1: content/layout changes. If the <iframe> is itself inside a display:none iframe, will we just not enter this code? Otherwise, seems like you need to null-check GetShell(). Also, that should probably be GetCurrentDoc(), not OwnerDoc(). And possibly null-check that too.... I don't understand the EnsureMessageManager change. Is that not usually called on random content subframes or something? Please document the new method on nsFrameLoader. Do we really want the LoadSrc to be completely async? Is doing it off a scriptrunner not good enough? If so, why not? Timothy or roc should review the view/widget bit. I'd like smaug to take a look over this too...

Comment on attachment 613666 [details] [diff] [review] Part 1: content/layout changes. What is the reason for the presshell change?

> I don't understand the EnsureMessageManager change. Is that not usually called on random content > subframes or something? > nsCOMPtr<nsIDOMChromeWindow> chromeWindow = >- do_QueryInterface(mOwnerContent->OwnerDoc()->GetWindow()); >- NS_ENSURE_STATE(chromeWindow); >+ do_QueryInterface(OwnerDoc()->GetWindow()); >+ if (!chromeWindow) { >+ nsCOMPtr<nsIDocShell> rootDocShell = GetRootDocShell(OwnerDoc()); >+ nsCOMPtr<nsIDOMWindow> rootWindow = do_GetInterface(rootDocShell); >+ chromeWindow = do_GetInterface(rootWindow); >+ } Before, we assumed that mOwnerContent->OwnerDoc() was chrome. But now it's not, so we have to walk up to the root docshell to find chrome. > Do we really want the LoadSrc to be completely async? Is doing it off > a scriptrunner not good enough? If so, why not? At one point during my testing, a script runner was not good enough. But I'm having some difficulty re-creating whatever didn't work. I'll get back to you. > What is the reason for the presshell change? >@@ -5079,11 +5079,10 @@ LayerManager* PresShell::GetLayerManager() > { > NS_ASSERTION(mViewManager, "Should have view manager"); > >- nsIView* rootView = mViewManager->GetRootView(); >- if (rootView) { >- if (nsIWidget* widget = rootView->GetWidget()) { >- return widget->GetLayerManager(); >- } >+ nsIWidget *widget; >+ mViewManager->GetRootWidget(&widget); >+ if (widget) { >+ return widget->GetLayerManager(); > } Without it, I get a null widget. I don't pretend to understand what's going on here, but the conversation I had was: > <jlebar> Does a content docshell's presshell's root view correspond to the view > of the root docshell? I'm trying to get a widget pointer, but the "root view" > I get doesn't have a widget. > <roc> you want nsIViewManager::GetRootWidget probably > <roc> or better still, nsIFrame::GetNearestWidget I used GetRootWidget because that seemed to correspond better to what the original code was doing.

> At one point during my testing, a script runner was not good enough. But I'm having some difficulty > re-creating whatever didn't work. I'll get back to you. Using a script runner seems to work fine, in desktop Linux Firefox and desktop Linux b2g. I'm not going to complain!

(In reply to Justin Lebar [:jlebar] from comment #56) > Without it, I get a null widget. I don't pretend to understand what's going > on here, but the conversation I had was: What is the code that calls GetLayerManager trying to do? And where is it?

(In reply to Timothy Nikkel (:tn) from comment #58) > (In reply to Justin Lebar [:jlebar] from comment #56) > > Without it, I get a null widget. I don't pretend to understand what's going > > on here, but the conversation I had was: > > What is the code that calls GetLayerManager trying to do? And where is it? Does this help? #0 PresShell::GetLayerManager #1 mozilla::layout::RenderFrameParent::GetLayerManager #2 mozilla::layout::RenderFrameParent::AllocPLayers #3 mozilla::layout::PRenderFrameParent::OnMessageReceived The message received is, presumably, AllocPLayers. It happens right after we show() the child.

Comment on attachment 613668 [details] [diff] [review] Part 2: ipc changes. >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp > void > TabParent::SetOwnerElement(nsIDOMElement* aElement) > { >+ // Calling GetWidget will cause us to save the screen's DPI, if we can. >+ unused << GetWidget(); Please factor this out into a TryCacheDPI() helper, that internally tries to get the widget and save the DPI. > void > TabParent::LoadURL(nsIURI* aURI) > { >+ printf("TabParent::LoadURL(%s) called before Show(); ignoring LoadURL.\n", spec.get()); NS_WARNING or printf_stderr plz. > void > TabParent::Show(const nsIntSize& size) > { > // sigh >+ mShown = true; >+ printf("[TabParent] Show\n"); You can just nuke this. > already_AddRefed<nsIWidget> >-TabParent::GetWidget() const >+TabParent::GetWidget() With the TryCacheDPI() change you don't need to un-|const| this, right? >+ nsCOMPtr<nsIWidget> widget = frame->GetNearestWidget(); >+ if (widget && mDPI <= 0) { >+ mDPI = widget->GetDPI(); >+ } >+ (Prefer to move this into TryCacheDPI().)

(In reply to Justin Lebar [:jlebar] from comment #61) > (In reply to Timothy Nikkel (:tn) from comment #58) > > (In reply to Justin Lebar [:jlebar] from comment #56) > > > Without it, I get a null widget. I don't pretend to understand what's going > > > on here, but the conversation I had was: > > > > What is the code that calls GetLayerManager trying to do? And where is it? > > Does this help? > > #0 PresShell::GetLayerManager > #1 mozilla::layout::RenderFrameParent::GetLayerManager > #2 mozilla::layout::RenderFrameParent::AllocPLayers > #3 mozilla::layout::PRenderFrameParent::OnMessageReceived > > The message received is, presumably, AllocPLayers. It happens right after > we show() the child. What does the document hierarchy look like? Ie what document is mFrameLoader in and what is its parent document? Maybe you just want nsContentUtils::LayerManagerForDocument?

> Ie what document is mFrameLoader in and what is its parent document? We have chrome document content document (mozbrowser1.html) <iframe mozbrowser src="http://mozilla.org"> content document (mozilla.org) mFrameLoader is for the iframe, and OwnerDoc is mozbrowser1.html. > Maybe you just want nsContentUtils::LayerManagerForDocument? Yay, that works. (I see that we needed to walk the docshell hierarchy, which we weren't doing before...)

No longer any layout changes in this patch.

Interdiff part 1 v2 --> v3: $ interdiff <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=614264) <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=614362)

v2 --> v2.1: Remove unnecessary NULL out param.

Interdiff part 2 v1 --> v2.1: $ interdiff <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=613668) <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=614364)

Comment on attachment 614362 [details] [diff] [review] Part 1 v3 - content changes If you don't change RenderFrameParent::GetLayerManager then I don't see how this is going to work. Also comment 61 said it was the GetLayerManager call in RenderFrameParent::AllocPLayers that was problematic, not the one in nsFrameLoader::ShowRemoteFrame.

> Before, we assumed that mOwnerContent->OwnerDoc() was chrome. But now it's > not, so we have to walk up to the root docshell to find chrome. Yes, I get that. Do we not call that function on _other_ content subframes? _That_ was my question.

(In reply to Timothy Nikkel (:tn) from comment #70) > Comment on attachment 614362 [details] [diff] [review] > Part 1 v3 - content changes > > If you don't change RenderFrameParent::GetLayerManager then I don't see how > this is going to work. I do; it's just in the other patch. :) > Also comment 61 said it was the GetLayerManager call in > RenderFrameParent::AllocPLayers that was problematic, not the one in > nsFrameLoader::ShowRemoteFrame. They both need to be changed; I just didn't see ShowRemoteFrame until I fixed AllocPLayers.

Comment on attachment 614362 [details] [diff] [review] Part 1 v3 - content changes >+++ b/content/base/src/nsFrameLoader.cpp >@@ -907,16 +913,23 @@ nsFrameLoader::ShowRemoteFrame(const nsIntSize& size) >+ if (!mOwnerContent || >+ !mOwnerContent->GetCurrentDoc() || >+ !nsContentUtils::LayerManagerForDocument(mOwnerContent->GetCurrentDoc()).get()) { The need for .get() should have been a red flag here. This code leaks the layer manager. ;) >+ // This has to happen asynchronously to make OOP browser frames work. >+ // There's no harm in doing it asynchronously for in-process browser >+ // frames. So I just remembered why scriptrunner might not have worked before: we needed an nsIFrame. Maybe at this point we're flushing frames at some point in the load process, which is why it works now? r=me modulo the memory leak and an explanation about message manager stuff.

> The need for .get() should have been a red flag here. This code leaks the layer manager. ;) Is that fixable in already_AddRefed (outside this patch, of course)? Seems like if one of those gets destructed before it's assigned into a comptr, it should unref. That's certainly what I expected it would do! Maybe it's done this way as an optimization, real or imagined.

> Is that fixable in already_AddRefed Not easily, imo. Wrapping an nsCOMPtr<> around it should help here.

(In reply to Boris Zbarsky (:bz) from comment #71) > > Before, we assumed that mOwnerContent->OwnerDoc() was chrome. But now it's > > not, so we have to walk up to the root docshell to find chrome. > > Yes, I get that. Do we not call that function on _other_ content subframes? > _That_ was my question. It did cross my mind that your question was too easy. :) EnsureMessageManager bails if (!mIsTopLevelContent && !mRemoteFrame). So if I understand your question this time, I think the answer is no. :)

Yes, perfect. Thanks.

smaug, do you have an ETA for review here? This bug is blocking the followup work on making b2g multi-process.

It is next in my review queue

Comment on attachment 614362 [details] [diff] [review] Part 1 v3 - content changes >- // FIXME get error codes from child >- mRemoteBrowser->LoadURL(mURIToLoad); >+ if (mRemoteBrowserShown || ShowRemoteFrame(nsIntSize(0, 0))) { >+ // FIXME get error codes from child >+ mRemoteBrowser->LoadURL(mURIToLoad); >+ } >+ else { } else { > > // FIXME/bug 589337: Show()/Hide() is pretty expensive for > // cross-process layers; need to figure out what behavior we really > // want here. For now, hack. > if (!mRemoteBrowserShown) { >+ if (!mOwnerContent || >+ !mOwnerContent->GetCurrentDoc() || >+ !nsContentUtils::LayerManagerForDocument(mOwnerContent->GetCurrentDoc()).get()) { So isn't this leaking all the LayerManagers? Or do they have some bizarre addref/release setup? > >+static already_AddRefed<nsIDocShell> >+GetRootDocShell(nsIDocument *aDocument) >+{ >+ nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(aDocument->GetWindow()); >+ nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(webNav); >+ NS_ENSURE_TRUE(treeItem, NULL); Coding style says still nsnull >+ bool isBrowserFrame = false; >+ GetReallyIsBrowser(&isBrowserFrame); >+ if (isBrowserFrame) { >+ // This has to happen asynchronously to make OOP browser frames work. >+ // There's no harm in doing it asynchronously for in-process browser >+ // frames. >+ nsContentUtils::AddScriptRunner(new LoadSrcRunner(this)); I don't understand why this is needed. Frame loading starts already using a script runner. >+ } >+ else { } else {

> Coding style says still nsnull Have you been following the thread in dev.planning? roc changed it to nsnull a few days ago, but bsmedberg thinks it should be NULL. It was NULL until roc changed it. The consensus in the thread seems to be that we should use something from standard C++, which points towards NULL. But anyway, the fact that there's disagreement suggests to me we shouldn't be wasting times in reviews over it. > So isn't this leaking all the LayerManagers? Or do they have some bizarre addref/release > setup? Yes, it leaks. See comment 74. > Frame loading starts already using a script runner. Yeah, that's part of the magic in this patch. Is comment 7 sufficient explanation?

(In reply to Justin Lebar [:jlebar] from comment #81) > Yeah, that's part of the magic in this patch. Is comment 7 sufficient > explanation? No. You add a new scriptrunner, but there is already a scriptrunner which starts the loading. I don't understand why the new scriptrunner changes anything. If you need nsIFrame to be there, wouldn't it be better to start loading then the nsIFrame is actually created. Like when nsFrameLoader::Show is called.

> No. You add a new scriptrunner, but there is already a scriptrunner which starts the > loading. I don't understand why the new scriptrunner changes anything. You're totally right, it doesn't. It did, but either something changed, or one of my later changes fixed this too.

Pushed to birch for FF15. https://hg.mozilla.org/projects/birch/rev/dd82458147fd

Backed out in https://hg.mozilla.org/projects/birch/rev/cc409a6f9195 - mochitest-3 on every platform, timeout in test_browserFrame4/5/6/7.html (and thus an end to the suite), debug Linux/Linux64, a leak of a BasicContainerLayer and friends.

That worked well. The problem is that oop browser frames aren't pref'ed off on desktop. Instead, we rely on the fact that browser frames are pref'ed off. That works fine in general, but not for the test suite! I'll push to try this time.

This is the same refptr.get() mistake I made elsewhere. We're green on try after this.

Comment on attachment 617489 [details] [diff] [review] Part 3: Don't leak the layer manager in IPC code. landitlanditlandit

Landed on inbound as: https://hg.mozilla.org/integration/mozilla-inbound/rev/949cb1977ef9 Unfortunately had to be backed out due to compile failures on multiple platforms: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=949cb1977ef9 https://tbpl.mozilla.org/php/getParsedLog.php?id=11197414&tree=Mozilla-Inbound#error0 { ../../../dom/ipc/TabParent.cpp: In member function 'void mozilla::dom::TabParent::LoadURL(nsIURI*)': ../../../dom/ipc/TabParent.cpp:201: error: invalid conversion from 'int' to 'const char*' ../../../dom/ipc/TabParent.cpp:201: error: initializing argument 1 of 'nsPrintfCString::nsPrintfCString(const char*, ...)' make[6]: *** [TabParent.o] Error 1 } https://hg.mozilla.org/integration/mozilla-inbound/rev/99649652cb34

Bitrotted by bug 745659.

https://hg.mozilla.org/mozilla-central/rev/949cb1977ef9 https://hg.mozilla.org/mozilla-central/rev/99649652cb34 https://hg.mozilla.org/mozilla-central/rev/9d00516b0ad7