Closed Bug 749018 Opened 12 years ago Closed 12 years ago

Make OOP <iframe mozbrowser> pass current browser frame tests

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(5 files, 4 obsolete files)

Right now, the tests only pass with OOP disabled.

Weird stuff is going on, as usual...
smaug, can you explain this assertion in nsFrameMessageManager::nsFrameMessageManager()?

    NS_ASSERTION(mContext || (aChrome && !aParentManager) || aProcessManager,
                 "Should have mContext in non-global/non-process manager!");

mContext comes from nsFrameScriptExecutor's mCx variable.  mCx doesn't get set until InitTabChildGlobalInternal, which has to occur after this call to TabChildGlobal::TabChildGlobal() -- see TabChild::InitTabChildGlobal().  So mCx is null.  I don't think this is a process manager, so it makes sense that one is false.

So if mCx is null and aProcessManager==false, the only way this can be true is if aChrome.  But we're not chrome.

What's going on here?

Here's the full stack:

> [TabChild] SHOW (w,h)= (0, 0)
> [Child 25934] WARNING: nsWindow::GetNativeData not implemented for this type: file ../../../src/widget/xpwidgets/PuppetWidget.cpp, line 609
> ++DOCSHELL 0x1db2970 == 1 [id = 1]
> ++DOMWINDOW == 1 (0x1e1c078) [serial = 1] [outer = (nil)]
> [Child 25934] ###!!! ASSERTION: Should have mContext in non-global/non-process manager!: 'mContext || (aChrome && !aParentManager) || aProcessManager', file ../../../../src/content/base/src/nsFrameMessageManager.h, line 101
> nsFrameMessageManager (/home/jlebar/code/moz/ff-git/debug/content/base/src/../../../../src/content/base/src/nsFrameMessageManager.h:102)
> TabChildGlobal (/home/jlebar/code/moz/ff-git/debug/dom/ipc/../../../src/dom/ipc/TabChild.cpp:999)
> mozilla::dom::TabChild::InitTabChildGlobal() (/home/jlebar/code/moz/ff-git/debug/dom/ipc/../../../src/dom/ipc/TabChild.cpp:900)
> mozilla::dom::TabChild::RecvShow(nsIntSize const&) (/home/jlebar/code/moz/ff-git/debug/dom/ipc/../../../src/dom/ipc/TabChild.cpp:553)
> mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PBrowserChild.cpp:1091)
> mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PContentChild.cpp:1634)
> mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const&) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/AsyncChannel.cpp:495)
> mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/RPCChannel.cpp:436)
> 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::MessagePump::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:114)
> mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:230)
> 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)
> XRE_RunAppShell (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsEmbedFunctions.cpp:674)
> mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git/debug/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:215)
> 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)
> XRE_InitChildProcess (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsEmbedFunctions.cpp:517)
> main (/home/jlebar/code/moz/ff-git/debug/ipc/app/../../../src/ipc/app/MozillaRuntimeMain.cpp:81)
> __libc_start_main (/build/buildd/eglibc-2.13/csu/libc-start.c:258)
> _start (/home/jlebar/code/moz/ff-git/debug/dist/bin/plugin-container)
Does aGlobal mean it's the global child process MM, while aProcess means it's the global parent process MM?
The next assertion I hit (this one is intermittent but fatal) is in RenderFrameParent::BuildLayer:

  NS_ABORT_IF_FALSE(!shadowRoot || shadowRoot->Manager() == aManager,
                    "retaining manager changed out from under us ... HELP!");

I don't have a C++ stack (now that I'm looking for it, it's not happening anymore), but the JS stack is telling:

0 PageThumbs_capture() ["resource:///modules/PageThumbs.jsm":85]
    this = [object Object]
1 PageThumbs_captureAndStore() ["resource:///modules/PageThumbs.jsm":107]
    this = [object Object]
2 Thumbnails_capture() ["chrome://browser/content/browser.js":3307]
    this = [object Object]
3 anonymous() ["chrome://browser/content/browser.js":3318]
    this = [object Object]
4 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>

I'll disable page thumbnails and try to continue.
Before you ask:

> 3 anonymous() ["chrome://browser/content/browser.js":3318]

Yes, this is only a problem in desktop builds; it's not a problem in b2g builds.  But we don't run mochitests in b2g builds.
(In reply to Justin Lebar [:jlebar] from comment #2)
> Does aGlobal mean it's the global child process MM,
global parent MM.

> while aProcess means
> it's the global parent process MM?
Process manager either in child or parent
But hmm, interesting, is the E10s MM broken. Looks like I managed to do that in Bug 705651.
(In reply to Justin Lebar [:jlebar] from comment #3)
> The next assertion I hit (this one is intermittent but fatal) is in
> RenderFrameParent::BuildLayer:
> 
>   NS_ABORT_IF_FALSE(!shadowRoot || shadowRoot->Manager() == aManager,
>                     "retaining manager changed out from under us ... HELP!");
> 
I have no idea about this
Attached patch WIP v1 (obsolete) — Splinter Review
This gets through the first 5 browser frame tests, and gets halfway through the sixth.  This test and the ones following it will need some work to get around OOP iframes; they assume they can reach into the mozbrowser iframe.
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attached patch WIP v1.1 (obsolete) — Splinter Review
Right patch this time.
Attachment #619213 - Attachment is obsolete: true
Depends on: 749993
Upcoming WIP passes the tests, but has a random failure on my machine (roughly 1 in 25 runs).  We've seen this assertion before.

[Parent 31558] ###!!! ABORT: Must not ask for DPI before OwnerElement is received!: 'mDPI > 0', file ../../../src/dom/ipc/TabParent.cpp, line 633
mozilla::dom::TabParent::RecvGetDPI(float*) (/home/jlebar/code/moz/ff-git/debug/dom/ipc/../../../src/dom/ipc/TabParent.cpp:634)
mozilla::dom::PBrowserParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PBrowserParent.cpp:1435)
mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PContentParent.cpp:1755)
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-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:319)
MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:329)
MessageLoop::DoWork() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:426)
mozilla::ipc::DoWorkRunnable::Run() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:71)
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)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:110)
MessageLoop::RunInternal() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:209)
MessageLoop::RunHandler() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:202)
MessageLoop::Run() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:175)
nsBaseAppShell::Run() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/widget/xpwidgets/../../../src/widget/xpwidgets/nsBaseAppShell.cpp:191)
nsAppStartup::Run() (/home/jlebar/code/moz/ff-git/debug/toolkit/components/startup/../../../../src/toolkit/components/startup/nsAppStartup.cpp:295)
XREMain::XRE_mainRun() (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3780)
XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3857)
XRE_main (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3933)
do_main (/home/jlebar/code/moz/ff-git/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:190)
main (/home/jlebar/code/moz/ff-git/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:277)
__libc_start_main (/build/buildd/eglibc-2.15/csu/libc-start.c:258)
_start (/home/jlebar/code/moz/ff-git/debug/dist/bin/firefox-bin)
[Parent 31558] ###!!! ABORT: Must not ask for DPI before OwnerElement is received!: 'mDPI > 0', file ../../../src/dom/ipc/TabParent.cpp, line 633
Attached patch WIP v2 (obsolete) — Splinter Review
Passes all the tests browser-frame, has random failure above.
Attachment #619214 - Attachment is obsolete: true
Note to reviewer (and myself): I made nsFrameLoader::SetOwnerContent private because it could mess up the remote script logic, and there's no need for it to be public.
Attached patch WIP v3 (obsolete) — Splinter Review
This is ready for review, except for the crash above.
Attachment #619348 - Attachment is obsolete: true
Smaug, do you have any idea why we might have a race condition here?  cjones passed the buck to you.  :)

This has something to do with the frame loader initialization and widgets.  "Can't get DPI" means "I have no widget when the child asks for the DPI", I think.
The "before OwnerElement is received!" is the part I don't understand.  Not having that is why we can't find a widget.

Can someone briefly describe the expected setup process for FrameLoader in terms of its enclosing DOM?  It seems that we're getting ahead of ourselves in the cross-process case, trying to do something in the content process before the parent is ready.
> The "before OwnerElement is received!" is the part I don't understand.  Not having that is why we 
> can't find a widget.

Well...

nsFrameLoader::ShowRemoteFrame() calls TryRemoteBrowser() if !mRemoteBrowser.

And if TryRemoteBrowser sets mRemoteBrowser non-null, it then calls mRemoteBrowser->SetOwnerElement().

So unless mOwnerContent is null when we call SetOwnerElement, something else is going on.
That's a possibility, don't know if it can actually happen though.  bz, smaug?

Another possibility is that the <iframe mozbrowser> is being created and torn down very quickly, and the DPI request is just late.  MOZ_IPC_MESSAGE_LOG=1 would let you see that.
I think mOwnerContent isn't null.  So perhaps the assertion's message is wrong -- we have the owner content, but no widget.

Sorry for the big paste, but notice the *** a few lines down -- that's us setting the owner element to non-null.  Unless there are more objects at play here or something.

> BrowserElementParent - _init
> JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 170: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]
> [time:1335836590078204][PContentParent] Sending Msg_PreferenceUpdate([TODO])
> [time:1335836590078303][PContentChild] Received Msg_PreferenceUpdate([TODO])
> [time:1335836590082090][PContentParent] Sending Msg_PBrowserConstructor([TODO])
> *** Successfully set owner element to 0x3773208
> [time:1335836590082132][PBrowserParent] Sending Msg_Show([TODO])
> [time:1335836590082192][PContentChild] Received Msg_PBrowserConstructor([TODO])
> creating 1!
> [time:1335836590082221][PBrowserParent] Sending Msg_LoadRemoteScript([TODO])
> [time:1335836590082236][PBrowserParent] Sending Msg_LoadRemoteScript([TODO])
> [time:1335836590082244][PBrowserParent] Sending Msg_LoadRemoteScript([TODO])
> [time:1335836590082245][PBrowserChild] Received Msg_Show([TODO])
> [TabChild] SHOW (w,h)= (0, 0)
> [time:1335836590082256][PBrowserParent] Sending Msg_LoadRemoteScript([TODO])
> [time:1335836590082271][PBrowserParent] Sending Msg_LoadRemoteScript([TODO])
> [time:1335836590082287][PBrowserParent] Sending Msg_LoadRemoteScript([TODO])
> [time:1335836590082308][PBrowserChild] Sending Msg_NotifyIMEFocus([TODO])
> BrowserElementParent - Remote frame shown [xpconnect wrapped nsISupports @ 0x32c39b0 (native @ 0x3776ec0)]
> [time:1335836590083358][PBrowserParent] Sending Msg_LoadRemoteScript([TODO])
> [time:1335836590083395][PBrowserParent] Sending Msg_LoadURL([TODO])
> [time:1335836590083603][PBrowserParent] Received Msg_NotifyIMEFocus([TODO])
> [time:1335836590083614][PBrowserParent] Sending reply Reply_NotifyIMEFocus([TODO])
> [time:1335836590083673][PBrowserChild] Received reply Reply_NotifyIMEFocus([TODO])
> [time:1335836590083686][PBrowserChild] Sending Msg_PRenderFrameConstructor([TODO])
> [time:1335836590083706][PRenderFrameChild] Sending Msg_PLayersConstructor([TODO])
> [time:1335836590083893][PBrowserParent] Received Msg_PRenderFrameConstructor([TODO])
> [time:1335836590083936][PRenderFrameParent] Received Msg_PLayersConstructor([TODO])
> [time:1335836590083952][PRenderFrameParent] Sending reply Reply_PLayersConstructor([TODO])
> [time:1335836590084068][PRenderFrameChild] Received reply Reply_PLayersConstructor([TODO])
> [Child 22193] WARNING: nsWindow::GetNativeData not implemented for this type: file ../../../src/widget/xpwidgets/PuppetWidget.cpp, line 609
> ++DOCSHELL 0x1de5da0 == 5 [id = 5]
> ++DOMWINDOW == 14 (0x1de67b8) [serial = 14] [outer = (nil)]
> [time:1335836590088202][PBrowserChild] Received Msg_LoadRemoteScript([TODO])
> [time:1335836590090004][PBrowserChild] Received Msg_LoadRemoteScript([TODO])
> [time:1335836590090185][PBrowserChild] Received Msg_LoadRemoteScript([TODO])
> [time:1335836590091330][PBrowserChild] Received Msg_LoadRemoteScript([TODO])
> [time:1335836590093211][PBrowserChild] Received Msg_LoadRemoteScript([TODO])
> [time:1335836590094776][PBrowserChild] Received Msg_LoadRemoteScript([TODO])
> [time:1335836590095111][PBrowserChild] Received Msg_LoadRemoteScript([TODO])
> BrowserElementChild - Starting up.
> [time:1335836590095879][PBrowserChild] Sending Msg_AsyncMessage([TODO])
> [time:1335836590096820][PBrowserChild] Received Msg_LoadURL([TODO])
> loading data:text/html,Outer%20iframe%20<iframe%20id="inner-iframe"></iframe>, 1
> [time:1335836590097200][PBrowserChild] Sending Msg_GetDPI([TODO])
> [time:1335836590135920][PBrowserParent] Received Msg_AsyncMessage([TODO])
> BrowserElementParent - recvHello [object HTMLIFrameElement @ 0x3776150 (native @ 0x3773170)]
> [time:1335836590136188][PBrowserParent] Received Msg_GetDPI([TODO])
> [Parent 22161] ###!!! ABORT: Must not ask for DPI before OwnerElement is received!: 'mDPI > 0', file ../../../src/dom/ipc/TabParent.cpp, line 633
> mozilla::dom::TabParent::RecvGetDPI(float*) (/home/jlebar/code/moz/ff-git/debug/dom/ipc/../../../src/dom/ipc/TabParent.cpp:634)
> mozilla::dom::PBrowserParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PBrowserParent.cpp:1435)
> mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (/home/jlebar/code/moz/ff-git/debug/ipc/ipdl/PContentParent.cpp:1755)
> 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-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:319)
> MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:329)
> MessageLoop::DoWork() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:426)
> mozilla::ipc::DoWorkRunnable::Run() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:71)
> 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)
> mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/glue/../../../src/ipc/glue/MessagePump.cpp:110)
> MessageLoop::RunInternal() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:209)
> MessageLoop::RunHandler() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:202)
> MessageLoop::Run() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/ipc/chromium/../../../src/ipc/chromium/src/base/message_loop.cc:175)
> nsBaseAppShell::Run() (/home/jlebar/code/moz/ff-git2/debug-jemalloc/widget/xpwidgets/../../../src/widget/xpwidgets/nsBaseAppShell.cpp:191)
> nsAppStartup::Run() (/home/jlebar/code/moz/ff-git/debug/toolkit/components/startup/../../../../src/toolkit/components/startup/nsAppStartup.cpp:295)
> XREMain::XRE_mainRun() (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3780)
> XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3857)
> XRE_main (/home/jlebar/code/moz/ff-git/debug/toolkit/xre/../../../src/toolkit/xre/nsAppRunner.cpp:3933)
> do_main (/home/jlebar/code/moz/ff-git/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:190)
> main (/home/jlebar/code/moz/ff-git/debug/browser/app/../../../src/browser/app/nsBrowserApp.cpp:277)
> __libc_start_main (/build/buildd/eglibc-2.15/csu/libc-start.c:258)
> _start (/home/jlebar/code/moz/ff-git/debug/dist/bin/firefox-bin)
> [Parent 22161] ###!!! ABORT: Must not ask for DPI before OwnerElement is received!: 'mDPI > 0', file ../../../src/dom/ipc/TabParent.cpp, line 633
Yeah, what's failing here is

  nsIFrame *frame = content->GetPrimaryFrame();

content is not null; frame is null.
OK.  It seems plausible that we shouldn't be displaying remote content until the parent side has a frame.  That hurts load parallelism though, but maybe we need to do this to ~respect DOM semantics.

bz, smaug: some guidance here would be appreciated.  Otherwise jlebar and I will just hack something up :).
Well, content may not always have a frame.

So, what does the failing test do?

(And yes, we should support also e10s frames which don't have a presentation)
Aha, if I do

  iframe.mozbrowser = true;
  iframe.src = 'data:text/html,Hello';
  iframe.style.display = 'none';
  document.body.appendChild(iframe);

it crashes every time.  (The test which crashes randomly is missing the display:none.)

Looking at the code, nsFrameLoader::ReallyStartLoadingInternal tries to show the remote frame, and then does mRemoteBrowser->LoadURL only if the show succeeded.

Perhaps we should make show() fail if there's no frame, but load the URL anyway.
Attachment #619352 - Attachment is obsolete: true
Attached patch Part 1, v1Splinter Review
This doesn't fail randomly on my machine, so I'm going to declare (provisional) victory.

To fix the DPI issue, I used nsContentUtils::WidgetForDocument and passed the ownerdoc of the frame element.  If that's not strong enough, perhaps we should just make a static DPI getter -- all the widget implementations I looked at return a system-global DPI.

Alternatively, we could return a fake DPI when we have no widget.  But it appears that whoever calls GetDPI caches the result, so that's not ideal.

We can't check this in until Gaia works with OOP mozbrowser (at the very least, it accesses contentwindow, which won't work anymore) and until OOP mozbrowser works on the device.

(r?cjones on the ipc change)
Attachment #619913 - Flags: review?(jones.chris.g)
Attachment #619913 - Flags: review?(bugs)
Depends on: 666693
> That hurts load parallelism though

Why?  Subframe content should load and run scripts and whatnot, but just not _render_ if the <iframe> has no nsIFrame.
(In reply to Justin Lebar [:jlebar] from comment #23)
> Created attachment 619913 [details] [diff] [review]
> Patch v1
> 
> We can't check this in until Gaia works with OOP mozbrowser (at the very
> least, it accesses contentwindow, which won't work anymore) and until OOP
> mozbrowser works on the device.
> 

This can be pref'd off, right?  We can and should absolutely land this pref'd off.  There's a lot of followup work that depends on it.
This patch is not structured to be pref'ed off.  That's intentional.

It's work to make this transparently pref'able.  At the moment, a page can currently tell whether an iframe is OOP.  OOP iframes have null contentWindow's, and don't send some events (e.g. 'load') to the parent.  There are probably other things.

If OOP mozbrowser is the future, then I don't think we should waste our time maintaining in-process mozbrowser or trying to make it look like OOP.

I think a better approach would be to land this on a branch in b2g-mc and do follow-up work there.  That way, nobody is blocked, we can easily collaborate, and we don't have the false fantasy that both in- and out-of-process mozbrowser is supported.

Note that we probably can't turn this on until we have support for indexed-db with multiple content processes; b2g dies at startup with this patch due to lack of indexed-db support.  I'm told this is a large, currently unowned project.
Is there something hard about pref'ing this on/off, vs. the patch not being structured to do so?

OOP isn't fully supported on all Tier Is yet.  If we make this OOP only, we can only realistically test in b2g and linux-gtk, basically.

IDB works across a single content process.  What problem are you seeing here?  Is b2g dying if we try to spawn a second content process?
> Is there something hard about pref'ing this on/off, vs. the patch not being structured 
> to do so?

The issue is that OOP mozbrowser is not identical to in-process mozbrowser.  By providing a preference, we're exposing this difference (or committing to make it go away).  I do not think that is useful.

> OOP isn't fully supported on all Tier Is yet.  If we make this OOP only, we can only 
> realistically test in b2g and linux-gtk, basically.

What exactly can't we test without supporting OOP on other Tier I's (let's just say, Mac)?

We obviously can't test OOP mozbrowser without supporting OOP on Mac.  And like I said, it's nontrivial to make non-OOP mozbrowser appear identical to OOP mozbrowser, and it will be nontrivial to maintain that invariant going forward.  So we can't test how b2g will run with OOP mozbrowser by substituting in-process mozbrowser.

So I don't understand why maintaining in-process mozbrowser is more important than simply getting things to work on mac.  We're going to need to do that eventually, before we switch to OOP mozbrowser for b2g, unless we commit to maintaining both an in-process and an OOP version of the API, and to making them appear identical.

> IDB works across a single content process.  What problem are you seeing here?  Is b2g 
> dying if we try to spawn a second content process?

AIUI, IDB may only be accessed from one process.  With this patch, we have two processes accessing IDB: The parent and the child.  This isn't a matter of "content" versus "chrome" processes -- the parent runs some small amount of chrome last time I looked, but the parent also runs content.

Maybe we could solve this by forcing everything into one process; I haven't looked into it.  (That would kind of defeat the purpose of this work, but it would let us move forward.)


I feel a lot of pressure to land specifically on m-c, but I don't understand why that's important.  As far as I can tell, landing on a branch is functionally equivalent to landing pref'ed off on m-c.  And we can land on a branch and start collaborating *now*, without waiting for reviews, test runs, etc., which will surely take days.
I'll see what I can do about getting in-process browser frame to resemble OOP browser-frame, but I still think that if you want this immediately, we should land it on a branch as-is.
(In reply to Justin Lebar [:jlebar] from comment #28)
> > Is there something hard about pref'ing this on/off, vs. the patch not being structured 
> > to do so?
> 
> The issue is that OOP mozbrowser is not identical to in-process mozbrowser. 
> By providing a preference, we're exposing this difference (or committing to
> make it go away).  I do not think that is useful.
> 

The ways in which they're different are off-limits to in-process code.  We could make the IP version look like OOP, but TBH I don't care that much.  This is the way we did dev for xul-fennec and firefox-e10s.

> > OOP isn't fully supported on all Tier Is yet.  If we make this OOP only, we can only 
> > realistically test in b2g and linux-gtk, basically.
> 
> What exactly can't we test without supporting OOP on other Tier I's (let's
> just say, Mac)?
> 
> We obviously can't test OOP mozbrowser without supporting OOP on Mac.  And
> like I said, it's nontrivial to make non-OOP mozbrowser appear identical to
> OOP mozbrowser, and it will be nontrivial to maintain that invariant going
> forward.  So we can't test how b2g will run with OOP mozbrowser by
> substituting in-process mozbrowser.
> 
> So I don't understand why maintaining in-process mozbrowser is more
> important than simply getting things to work on mac.  We're going to need to
> do that eventually, before we switch to OOP mozbrowser for b2g, unless we
> commit to maintaining both an in-process and an OOP version of the API, and
> to making them appear identical.
> 

Cross-process OS X and windows is around Tier III right now.  There are many things that can break that code that are miles away from mozbrowser.  And people will break it intentionally if it gets in the way of other higher-priority work.  That's going to affect dev time for people who prefer testing on those OS's.

I feel like we might be talking past each other a bit here, though.

> > IDB works across a single content process.  What problem are you seeing here?  Is b2g 
> > dying if we try to spawn a second content process?
> 
> AIUI, IDB may only be accessed from one process.  With this patch, we have
> two processes accessing IDB: The parent and the child.  This isn't a matter
> of "content" versus "chrome" processes -- the parent runs some small amount
> of chrome last time I looked, but the parent also runs content.
> 

My understanding was that there was a hacky crappy cross-process impl that only worked with one content process.

BTW, the terminology we've used in the past is "chrome process" for the main process, and "content process" for the child processes.  It's probably not the best but you'll see that all over the place.

> I feel a lot of pressure to land specifically on m-c, but I don't understand
> why that's important.  As far as I can tell, landing on a branch is
> functionally equivalent to landing pref'ed off on m-c.  And we can land on a
> branch and start collaborating *now*, without waiting for reviews, test
> runs, etc., which will surely take days.

Landing on a branch raises the bar considerably higher for continued dev, and brings in merge-back mechanics which is hard (especially when code lands on branch without review).  I'm not summarily against it but if we can get this patch on m-c and start on followup work within a few days, without branching, that feels like a better solution to me.
If we don't care at all about maintaining similar semantics between oop and non-oop, this is simple enough to do.  I'll have the patch up later today.
We do, however I would prefer to land this first, since it's more important atm.
(In reply to Justin Lebar [:jlebar] from comment #31)
> If we don't care at all about maintaining similar semantics between oop and
> non-oop, this is simple enough to do.  I'll have the patch up later today.

Ah, not quite so simple.  Only the topmost content frame in a process gets a MM.  So to do mozbrowser in-process, we have to change things.  I'll see what I can do.

In the meantime, parallel work can happen on top of the patch I've posted, or by pulling [1].  This basically works on Linux (OOP), so nobody should be blocked waiting for me to get it to work on Mac (in-process).

[1] https://github.com/jlebar/mozilla-central/tree/mozbrowser-crossprocess-2
Comment on attachment 619913 [details] [diff] [review]
Part 1, v1

>diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js

> // Enable browser frames, but not OOP.
> pref("dom.mozBrowserFramesEnabled", true);
> pref("dom.mozBrowserFramesWhitelist", "http://homescreen.gaiamobile.org,http://browser.gaiamobile.org");
>-pref("dom.ipc.tabs.disabled", true);
>+pref("dom.ipc.tabs.disabled", false);
> 

This doesn't match up with the comment anymore.  But we should leave
this off for now while we stand up content processes on gonk.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp

> void
> TabParent::TryCacheDPI()

>+  if (!widget && mFrameElement) {
>+    // Get just any widget; it doesn't matter for the purposes of DPI.

Technically it does ... we can have a window that lives on one monitor
with DPI x, and another window that lives on a second monitor with DPI
y.

In fact, my FF installation is running in this configuration as I type
this :).

But I'm very much OK with this for now.  Please file follow-up and
leave "FIXME/bug XXXXXX" breadcrumb.

>diff --git a/dom/ipc/jar.mn b/dom/ipc/jar.mn

> toolkit.jar:
>         content/global/test-ipc.xul (test.xul)
>         content/global/remote-test-ipc.js (remote-test.js)
>+	content/global/BrowserElementChild.js (../base/BrowserElementChild.js)

\t snuck in.

Didn't review the browser-frame or test changes (i.e., about 95% of
this patch ;).
Attachment #619913 - Flags: review?(jones.chris.g) → review+
> Technically it does ... we can have a window that lives on one monitor
> with DPI x, and another window that lives on a second monitor with DPI
> y.

Not all of our widget code is smart enough to understand that; at least, it doesn't appear our gtk2 nsWindow is.  The win32 code might be...although who knows what happens when the window is half on one screen and half on another.

Anyway, that code will grab the closest widget in the hierarchy, not a random one.  I'll update the comment, but I don't think it needs a follow-up, at least as I understand things.
The X server lies about DPI, no point asking it.

But again, like I said, this impl is fine for now :).
Comment on attachment 619913 [details] [diff] [review]
Part 1, v1


>+
>+  /**
>+   * The element which owns this frame loader.
>+   *
>+   * For example, if this is a frame loader for an <iframe>, this attribute
>+   * returns the iframe element.
>+   */
>+  readonly attribute nsISupports ownerElement;
nsIDOMElement


>+++ b/dom/base/BrowserElementChild.js
>@@ -0,0 +1,107 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+"use strict";
>+
>+let Cu = Components.utils;
>+let Ci = Components.interfaces;
>+let Cc = Components.classes;
>+
>+function debug(msg) {
>+  dump("BrowserElementChild - " + msg + "\n");
>+}
>+
>+function sendAsyncMsg(msg, data) {
>+  sendAsyncMessage('browser-element-api:' + msg, data);
>+}
>+
>+/**
>+ * The BrowsserElementChild implements one half of <iframe mozbrowser>.
ss


>+ * (The other half is, unsurprisingly, BrowserElementParent.)
>+ *
>+ * This script is injected into an <iframe mozbrowser> via
>+ * nsIMessageManager::LoadFrameScript().
>+ *
>+ * Our job here is to listen for events within this frame and bubble them up to
>+ * the parent process.
>+ */
>+
>+function BrowserElementChild() {
>+  this._init();
>+};
>+
>+BrowserElementChild.prototype = {
>+  _init: function() {
>+    debug("Starting up.");
>+    sendAsyncMsg("hello");
Really? "hello"


>+
>+    // The docShell keeps a weak reference to the progress listener, so we need
>+    // to keep a strong ref to it ourselves.
>+    docShell.QueryInterface(Ci.nsIWebProgress)
>+            .addProgressListener(this._progressListener,
>+                                 Ci.nsIWebProgress.NOTIFY_LOCATION |
>+                                 Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
>+
>+    addEventListener('DOMTitleChanged',
>+                     this._titleChangedHandler.bind(this),
>+                     /* useCapture = */ false,
You probably do want useCapture to be true


>+                     /* wantsUntrusted = */ false);
>+  },
>+
>+  _titleChangedHandler: function(e) {
>+    debug("Got titlechanged: (" + e.target.title + ")");
>+    var win = e.target.defaultView;
>+    if (win.parent != win) {
>+      // Not a top-level window; ignore it.
>+      debug("not top-level window.");
>+      return;
>+    }
>+
>+    sendAsyncMsg('titlechange', e.target.title);
>+  },
>+
>+  _progressListener: {
>+    QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
>+                                           Ci.nsISupportsWeakReference,
>+                                           Ci.nsISupports]),
>+    _seenLoadStart: false,
>+
>+    onLocationChange: function(webProgress, request, location, flags) {
>+      // We get progress events from subshells here, which is kind of weird.
>+      if (webProgress != docShell) {
>+        return;
>+      }
>+
>+      // Ignore locationchange events which occur before the first loadstart.
>+      // These are usually about:blank loads we don't care about.
>+      if (!this._seenLoadStart) {
>+        return;
>+      }
>+
>+      sendAsyncMsg('locationchange', location.spec);
>+    },
>+
>+    onStateChange: function(webProgress, request, stateFlags, status) {
>+      if (webProgress != docShell) {
>+        return;
>+      }
>+
>+      if (stateFlags & Ci.nsIWebProgressListener.STATE_START) {
>+        this._seenLoadStart = true;
>+        sendAsyncMsg('loadstart');
>+      }
>+
>+      if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
>+        sendAsyncMsg('loadend');
>+      }
>+    },
>+
>+    onStatusChange: function(webProgress, request, status, message) {},
>+    onProgressChange: function(webProgress, request, curSelfProgress,
>+                               maxSelfProgress, curTotalProgress, maxTotalProgress) {},
>+    onSecurityChange: function(webProgress, request, aState) {}
>+  },
>+};
>+
Please put all this inside a closure.


>+++ b/dom/ipc/TabChild.cpp
...
>--- a/dom/ipc/TabChild.h

This part is from my patch.
Attachment #619913 - Flags: review?(bugs) → review+
FWIW the tests pass with OOP everywhere except Windows.

https://tbpl.mozilla.org/?tree=Try&rev=90a1602284b5

But if we're landing this on m-c, we need a way to disable it, because Gaia currently won't work with OOP (e10s indexed-db, touching iframe.contentWindow, probably other things), so I'll keep working on that.

>+  readonly attribute nsISupports ownerElement;
nsIDOMElement

The owner element might be <xul:browser> -- I was concerned that wouldn't be an nsIDOMElement, but ehsan tells me that's wrong, so this sounds good.

> Please put all this inside a closure.

FYI I'll have to keep a ref to the progress listener, since the docshell keeps only a weak ref.  I dunno if that changes your suggestion; I'm cool with it either way.
Olli, in non-OOP, from within BrowserElementChild.js (injected via loadFrameScript), it appears that addEventListener on the global is not adding a listener to the chrome event handler, because I don't hear DOMTitleChanged.

Perhaps I'm passing in the wrong jscontext to the nsFrameMessageManager constructor?  Is the current cx the right one?
Oh, right. This gets tricky.
Events propagate from content windows to message manager between chrome and content.
So, nsPIDOMWindow::GetParentTarget should return your new message manager, but that message manager
should propagate the event to the normal message manager.
> So, nsPIDOMWindow::GetParentTarget should return your new message manager, but that message manager
> should propagate the event to the normal message manager.

Okay, I think I'm setting the mm correctly in UpdateParentTarget().  But could you give me more details about this PreHandleEvent() business that I'm to use to forward from the mozbrowser mm to the "normal" mm?

With only the parent target set, I'm still not getting DOMTitleChange events.  I don't see why I'd need correct forwarding to make that work, so maybe something else is wrong too.
Hmm, DOMTitleChange is dispatched to chrome only.
I guess we want the event target chain to be
contentDOM->window->newMM(chrome-only-events get dispatched here)->oldMM->chromeDOM->chromeWindow->windowRoot

So, in nsContentUtils::DispatchChromeEvent you could change 
nsIDOMEventTarget* piTarget = aDoc->GetWindow()->GetChromeEventHandler(); to
nsIDOMEventTarget* piTarget = aDoc->GetWindow()->GetParentTarget();
and remove the frameloader hackery.
(GetParentTarget() should do the right thing anyway here. The frameloader hackery just
predates GetParentTarget() IIRC)


Another place to change is
if (aEvent->flags & NS_EVENT_FLAG_ONLY_CHROME_DISPATCH) {
in nsEventDispatcher::Dispatch
Attachment #619913 - Attachment description: Patch v1 → Part 1, v1
I made the tests run in-process on Windows and OOP everywhere else.  In-process fails the window.top test, as expected, so I disabled it.
I'm uploading these because I suspect I may have misunderstood the end of comment 37 ("Please put all this inside a closure.").

Is this what you wanted?  It does not seem to be an improvement to me; it's just moving all the logic into the init() function.
Attachment #620851 - Flags: feedback?(bugs)
Try run for f4254c590461 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f4254c590461
Results (out of 107 total builds):
    exception: 8
    success: 6
    warnings: 28
    failure: 65
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-f4254c590461
Try run for 5d1daee13879 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5d1daee13879
Results (out of 216 total builds):
    exception: 3
    success: 58
    warnings: 67
    failure: 88
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-5d1daee13879
Try run for 45a7ba12b173 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=45a7ba12b173
Results (out of 216 total builds):
    success: 149
    warnings: 26
    other: 41
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-45a7ba12b173
 Timed out after 12 hours without completing.
Try run for 45a7ba12b173 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=45a7ba12b173
Results (out of 216 total builds):
    success: 152
    warnings: 27
    other: 37
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-45a7ba12b173
 Timed out after 12 hours without completing.
Try run for 45a7ba12b173 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=45a7ba12b173
Results (out of 216 total builds):
    success: 153
    warnings: 27
    other: 36
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-45a7ba12b173
 Timed out after 12 hours without completing.
Try run for 88642b6915cf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=88642b6915cf
Results (out of 53 total builds):
    success: 48
    warnings: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-88642b6915cf
Comment on attachment 621109 [details] [diff] [review]
Part 1d, v1: Fix event parents

Can't keep my own patch numbers straight...
Attachment #621109 - Attachment description: Part 1e, v1: Fix event parents → Part 1d, v1: Fix event parents
Fyi, I tried to grab these patches to hack on some followup multi-process work, but (i) they've bitrotted significantly on m-c; and (ii) your git repo doesn't build against latest b2g ICS.
I pushed rebased patches to github.  I'm not going to update the patches here because that makes interdiff hard, and there were no functional changes; the bitrot was entirely in context.

I have no idea why gecko would build on tryserver-b2g but not against ICS, unless b2g's gecko contains commits not in m-c.  (I thought we were no longer doing this?)  If so, you should cherry-pick the commits and place them atop b2g head, or otherwise merge the branch into b2g head.

Anyway, I'm checking out ICS to see for myself.
> Anyway, I'm checking out ICS to see for myself.

Someone is going to have to hold my hand next week through getting an ICS build up and running.  My SGSII is running the wrong firmware, etc.
Your repo was just missing an ICS build fix.  Rebasing to tip would have fixed that.

This didn't slow me down at all, no worries :).

(In reply to Justin Lebar [:jlebar] from comment #57)
> > Anyway, I'm checking out ICS to see for myself.
> 
> Someone is going to have to hold my hand next week through getting an ICS
> build up and running.  My SGSII is running the wrong firmware, etc.

Will you be in San Diego?
Comment on attachment 621109 [details] [diff] [review]
Part 1d, v1: Fix event parents

>+
>+  // If owner corresponds to an <iframe mozbrowser>, we'll have to tweak our
>+  // PreHandleEvent implementation.
>+  nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(mOwner);
>+  bool isBrowser = false;
>+  if (browserFrame) {
>+      browserFrame->GetReallyIsBrowser(&isBrowser);
>+  }
2 space indentation


> nsresult
> nsInProcessTabChildGlobal::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
> {
>   aVisitor.mCanHandle = true;
>-  aVisitor.mParentTarget = mOwner;
>+
>+  nsIDOMEventTarget* target = mOwner;
>+  if (mIsBrowserFrame && mOwner) {
>+      nsPIDOMWindow* innerWindow = mOwner->OwnerDoc()->GetInnerWindow();
>+      if (innerWindow) {
>+        target = innerWindow->GetParentTarget();
>+      }
>+  }
>+
>+  aVisitor.mParentTarget = target;
You don't want to set mParentTarget to target if innerWindow is null.
Web pages might get access to events from child frames.

Perhaps
  if (mIsBrowserFrame) {
    if (mOwner) {
      nsPIDOMWindow* innerWindow = mOwner->OwnerDoc()->GetInnerWindow();
      if (innerWindow) {
        aVisitor.mParentTarget = innerWindow->GetParentTarget();
      }
    }
  } else {
    aVisitor.mParentTarget = mOwner;
  }

>@@ -137,15 +137,16 @@ public:
> protected:
>   nsresult Init();
>   nsresult InitTabChildGlobal();
>   nsCOMPtr<nsIContentFrameMessageManager> mMessageManager;
>   nsCOMPtr<nsIDocShell> mDocShell;
>   bool mInitialized;
>   bool mLoadingScript;
>   bool mDelayedDisconnect;
>+  bool mIsBrowserFrame;
Please add some comment what this is used for.
Attachment #621109 - Flags: review?(bugs) → review+
Comment on attachment 620848 [details] [diff] [review]
Part 1b, v1: Make in-process mozbrowser work.


>+  } else if (OwnerIsBrowserFrame()) {
>+    // XXX are these params right?
Looks right to me.
But why not reuse the code below?

>+  /**
>+   * Is the given window a top-level <iframe mozbrowser>?
>+   *
>+   * If we're OOP, a top-level window will have win.parent == win.  (Other,
>+   * non-mozbrowser windows will of course also have win.parent == win, but
>+   * BrowserElementChild won't be instantiated for those windows.)
>+   *
>+   * If we're in-process, a top-level mozbrowser window will have
>+   * nsIDocShell::IsBrowserFrame set to true.
>+   */
>+  _isTopLevelBrowserWindow: function(win) {
>+    if (win.parent == win) {
>+      return true;
>+    }
>+
>+    var docshell = win.QueryInterface(Ci.nsIInterfaceRequestor)
>+                      .getInterface(Ci.nsIWebNavigation)
>+                      .QueryInterface(Ci.nsIDocShell);
>+    return docshell && docshell.isBrowserFrame;

Couldn't you just do
return win == content && docShell && docShell.isBrowserFrame;

>+++ b/dom/tests/mochitest/browser-frame/browserFrameHelpers.js
>@@ -101,11 +101,20 @@ const browserFrameHelpers = {
> browserFrameHelpers.origEnabledPref = browserFrameHelpers.getEnabledPref();
> browserFrameHelpers.origWhitelistPref = browserFrameHelpers.getWhitelistPref();
> browserFrameHelpers.origOOPDisabledPref = browserFrameHelpers.getOOPDisabledPref();
> browserFrameHelpers.origPageThumbsEnabledPref = browserFrameHelpers.getPageThumbsEnabledPref();
> 
> // Disable tab view; it seriously messes us up.
> browserFrameHelpers.setPageThumbsEnabledPref(false);
> 
>+// OOP must be disabled on Windows; it doesn't work there.  Enable it
>+// everywhere else.
Er what? o_O

> 
>   iframe1.src = 'data:text/html,<html><head><title>Title</title></head><body></body></html>';
>-  iframe2.src = 'data:text/html,<html><head><title>BAD TITLE</title></head><body></body></html>';
>-  iframe3.src = 'data:text/html,<html><head><title>SHOULD NOT GET EVENT</title></head><body></body></html>';
>+  //iframe2.src = 'data:text/html,<html><head><title>BAD TITLE</title></head><body></body></html>';
>+  //iframe3.src = 'data:text/html,<html><head><title>SHOULD NOT GET EVENT</title></head><body></body></html>';
?
Attachment #620848 - Flags: review?(bugs) → review-
Attachment #620851 - Flags: feedback?(bugs) → feedback+
>>+// OOP must be disabled on Windows; it doesn't work there.  Enable it
>>+// everywhere else.
>
> Er what? o_O

What's your question, exactly?  See e.g. comment 30.
Ok, comment 30 explained that one.
> Couldn't you just do
> return win == content && docShell && docShell.isBrowserFrame;

No, because OOP browser frame docshells don't have isBrowserFrame set.

I guess we could set docshell.isBrowserFrame on initialization of the BrowserFrameChild.  We could even set it as an expando, so we could get rid of the code in docshell itself...
I really don't like the closure change in part 1c, by the way.  It puts all the code in the init function.  We might as well not have a class, and just have one big init function with some global variables.

What were you hoping to accomplish by this change (comment 37)?
It appears that your patches here dropped 

bool
TabChild::InitTabChildGlobal()
{
...
  scope->Init();

which is most likely the cause of bug 752408.  Was that intentional?
No, it just looks like a rebase mistake.  I've fixed it.
(In reply to Justin Lebar [:jlebar] from comment #64)
> I really don't like the closure change in part 1c, by the way.  It puts all
> the code in the init function.  We might as well not have a class, and just
> have one big init function with some global variables.
> 
> What were you hoping to accomplish by this change (comment 37)?

The closure thing makes sure that if other frame scripts get loaded, they don't override any
property names in the global.
So somehow, in-process browser frames causes us to fire all events twice, on Mac.  Not on Linux.  I'm not sure yet what's going on here.

The fact that this bug is possible suggests that we should be running the in- and out-of-process tests on all platforms.  That's a pain, but it would have let me catch this much earlier.
> So somehow, in-process browser frames causes us to fire all events twice, on Mac.

Oh, the problem was that the old BrowserElementAPI.js, though deleted from my tree, was hanging out in my objdir, and helpfully firing events.  So this works now, hooray!

I'll post a patch for smaug in a few minutes.  I didn't rewrite all the tests to run both in- and out-of-process on all platforms, but I think we can handle that as a follow-up.  We're still getting coverage of both in-process and OOP at the moment (tests run in-process on Windows and OOP elsewhere), so I think it's OK.
I think this addresses the latest review comments.  (Good call on _isTopLevelWindow, by the way -- that should just be |win == content|.)

I also updated the git branch, for those following along at home.
Attachment #621893 - Flags: review?(bugs)
Attachment #621893 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/f6f8d92907b5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
No longer depends on: 666693
Depends on: 666693
You need to log in before you can comment on or make changes to this bug.