669698 - Implement "linked remote browser" to load the inspector in the same content process as the content it inspects

Status: ASSIGNED

(Core :: DOM: Core & HTML, defect)

Description

[Benjamin Smedberg]

The devtools plan is that although webconsole and debugging will occur remotely, the inspector panel (tree view) needs to be implemented in the same process as the content it inspects.

My current thought is something like:

<xul:browser type="content-primary" remote="true" id="idofremotebrowser"/>
...
<xul:iframe type="chrome" remote="true" related="idofremotebrowser" src="chrome://inspector/content/inspectorpanel.xul" />

And the content scripts for the iframe would have the following extra properties: * relatedContent (the window of the content specified by "linked")
* relatedDocshell (the docshell of the content specified by "linked")

Comment 1

[Rob Campbell [:rc] (:robcee)]

This is a good start.

Currently that iframe lives in a xul:panel. Probably not an issue.

What *may* be an issue is that the code that populates this iframe doesn't live in the iframe itself. It's a jsm imported into chrome.

We may need a way to host these jsms in the content process as well and give them access to both the content process' browser document and the iframe's document.

Comment 2

[Benjamin Smedberg]

That's not a problem, you can use JSMs from the content script to hook it all up.

Comment 3

[Benjamin Smedberg]

Olli, can you take this? This blocks the inspector/highlighter work that the devtools team needs to do in early Q4. Let me know if you can attach a date when you think it might be done, and I'll add it to the short-term goal list.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I should be able to do this next week, if not this weekend.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Oh, hmm, we don't support message managers for non-remote chrome browsers.
But I guess I could change that, so that in case of
"inspection" browsers, there could be mm.

Also, so far we haven't supported chrome remote browsers.


I guess the syntax/API needs to look like:

<browser type="content-primary"/>
<browser type="chrome" forcemessagemanager="true" needsrelated="true">

(The fact that browsers are remote or not doesn't change anything.
 It is just that they both need to be remote="true" or both need to be remote="false".
 Also, it doesn't matter whether one uses <browser> or <iframe>.)

needsrelated attribute is need to prevent automatic process and docshell creation.

Then, when needed one does in the code something like
var browsers = document.getElementsByTagName("browser");
browsers[1].relatedBrowser = browsers[0];

Setting .relatedBrowser ensures that both browsers use the same process.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #5) 
> Then, when needed one does in the code something like
> var browsers = document.getElementsByTagName("browser");
> browsers[1].relatedBrowser = browsers[0];

Hrm, this may need to be
browsers[1].frameLoader.relatedBrowser = browsers[0].frameLoader;

Comment 7

[Benjamin Smedberg]

There's no particular reason why these browsers need to be type="chrome", is there? Wouldn't a type="content" browser work just fine, as long as it isn't content-primary?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I thought there would be some js running in the inspector frame, and it should be able to access
the related frame. If the inspector doesn't run using chrome privileges, that access would be
blocked.

(And btw, type="chrome" should be easy to implement.)

Comment 9

[Benjamin Smedberg]

The inspector is running with chrome privileges, but as far as I know that's no reason why we couldn't load it into a content-type docshell.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Actually, inspector should use xul:iframe since xul:browser has already some
assumptions when and how message manager works.

I have WIP patch for this. Need to write some more tests and clean up the code a bit.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Uploaded to tryserver.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Rob, would be great if you could try the patch a bit.
Basically, create <xul:iframe needsrelated="true">, add it to document and
set its relatedFrame to point some frameloader
iframe.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.relatedFrame = _frame_loader_of_some_browser;
At that point the messagemanager of iframe is created and you can send scripts and add message listeners and send
messages. In the tabchildglobal of iframe relatedDocShell and relatedContent should now point to the other tab.

iframe is out-of-process if related browser is out-of-process, otherwise same-process.

Also, it should be possible to re-set .relatedFrame. That re-creates message manager.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Tryserver shows still some problem.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Linux64 only problems o_O

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 558269 [details] [diff] [review]
patch

New patch coming (without support for chrome docshells in content process).

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

The patch is still causing apparently content process crash in tryserver/linux64.
Can't reproduce locally.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 558443 [details] [diff] [review]
patch

Benjamin, what do you think about this approach?

Comment 20

[Benjamin Smedberg]

Comment on attachment 558443 [details] [diff] [review]
patch

I'd like bz or somebody to review this. I think the API is ok, but perhaps the devtools team should provide feedback on that.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Yes, anyone from devtools team, please give feedback.
This is the first step, we can sure add more to the API later.

Comment 22

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 558443 [details] [diff] [review]
patch

I *think* this API looks fine. I have a couple of comment nits for you, but otherwise, f+.

since you're in nsIFrameLoader.idl anyway:

   /**
    * With this event mode, it's the application's responsability to 
    * convert and forward events to the content process

change responsability->responsibility.

in nsFrameLoader.cpp:

+  // We don't fire any errors or throw exceptions if we're
+  // still waiting that .relatedFrame will be set.
+  if (NeedsRelated() && !mRelatedFrame) {

ITYM, "...if we're still waiting for .relatedFrame to be set".

thanks! :)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 558443 [details] [diff] [review]
patch

>+++ b/content/base/public/nsIFrameLoader.idl
>+  attribute nsIFrameLoader relatedFrame;

This needs documentation.  The interaction between LoadURI and the setter here is ... non-obvious at best.

Thank you for adding that builtinclass annotation!

>+++ b/content/base/public/nsIFrameMessageManager.idl
>+   * The current top level window in the related frame or null.

"current" makes it sounds like it's an inner window.  I assume it's actually an outer window, right?  Certainly the getter returns an outer window.

In that case, what makes it "current"?

Worth documenting whether "null" means no related frame or no window in it or either one.

>+   * The related docshell or null.

Again, document when null can happen.

>+++ b/content/base/src/nsFrameLoader.cpp
>+  // still waiting that .relatedFrame will be set.

 still waiting to be told what our related frame is.

>+  mShown = PR_TRUE;

Please document the mShown member.  And maybe call it mIsShowing?

>+nsFrameLoader::SetRelatedFrame(nsIFrameLoader* aRelated)

Why do we enforce that aRelated != this but not lack of other sorts of loops in the related-frame chain?

>+++ b/content/base/src/nsFrameLoader.h
>+  nsresult DestroyInternal(PRBool aReuse);

Document, please.  Especially the invariants aReuse values assume.

>+  bool NeedsRelated();

Document.

>+++ b/dom/ipc/PBrowser.ipdl
>+    RelatedFrame(PBrowser related);

Document.

>+++ b/dom/ipc/TabChild.h
>+    nsWeakPtr mRelatedFrame;

It would be good to document what you can ask for from that nsWeakPtr.  You seem to put a docshell in but ask for a webnavigation back out, then getInterface a docshell from that... which is confusing.  I suspect you want to change some part of that code too.

r=me with the above issues fixed.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Um, nice, the test case crashes now on Windows try.


Crash reason:  EXCEPTION_BREAKPOINT
Crash address: 0x6a0c111b

Thread 0 (crashed)
 0  mozalloc.dll!mozalloc_abort(char const * const) [mozalloc_abort.cpp:1131ea691369 : 77 + 0x0]
    eip = 0x6a0c111b   esp = 0x0030c804   ebp = 0x0030c8a4   ebx = 0x00000040
    esi = 0x6bb7e457   edi = 0x6bbba392   eax = 0x00000000   ecx = 0x6bbba4c8
    edx = 0x00000003   efl = 0x00000202
    Found by: given as instruction pointer in context
 1  xul.dll!NS_DebugBreak_P [nsDebugImpl.cpp:1131ea691369 : 345 + 0x9]
    eip = 0x6abf00eb   esp = 0x0030c810   ebp = 0x0030c8a4
    Found by: call frame info
 2  xul.dll!mozilla::layers::SurfaceDescriptor::MaybeDestroy(mozilla::layers::SurfaceDescriptor::Type) [PLayers.cpp:1131ea691369 : 326 + 0x17]
    eip = 0x6abb8899   esp = 0x0030cc34   ebp = 0x0030d0ec   ebx = 0x00000002
    Found by: call frame info
 3  xul.dll!mozilla::layers::SurfaceDescriptor::operator=(mozilla::layers::SurfaceDescriptor const &) [PLayers.cpp:1131ea691369 : 437 + 0x6]
    eip = 0x6abb895c   esp = 0x0030cc4c   ebp = 0x0030d0ec
    Found by: call frame info
 4  xul.dll!mozilla::layers::ShadowThebesLayerD3D10::Swap(mozilla::layers::ThebesBuffer const &,nsIntRegion const &,mozilla::layers::OptionalThebesBuffer *,nsIntRegion *,mozilla::layers::OptionalThebesBuffer *,nsIntRegion *) [ThebesLayerD3D10.cpp:1131ea691369 : 485 + 0xa]
    eip = 0x6ac4ec7f   esp = 0x0030cc64   ebp = 0x0030cc78   ebx = 0x0de86820
    Found by: call frame info
 5  xul.dll!mozilla::layers::ShadowLayersParent::RecvUpdate(InfallibleTArray<mozilla::layers::Edit> const &,InfallibleTArray<mozilla::layers::EditReply> *) [ShadowLayersParent.cpp:1131ea691369 : 326 + 0x21]
    eip = 0x6ac51e5b   esp = 0x0030cc80   ebp = 0x0030d0e0
    Found by: call frame info
 6  xul.dll!mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const &,IPC::Message * &) [PLayersParent.cpp:1131ea691369 : 220 + 0x17]
    eip = 0x6ab98274   esp = 0x0030d158   ebp = 0x0030d174   ebx = 0xfffffffc
    Found by: call frame info

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

One way how to simplify Firebug's e10s adoption is utilizing the linked remote browser (just like the built-in inspector panel)

In fact the entire Firebug UI (except of the Firebug start button now available on Firefox toolbar and Firefox Web Developer menu overly) is embedded inside a <xul:browser> element, see the following overlay:

<!-- Firebug panel -->
<vbox id="appcontent">
    <splitter id="fbContentSplitter" collapsed="true"/>
    <vbox persist="height" id="fbMainFrame" collapsed="true">
        <browser id="fbMainContainer" flex="2" src="chrome://firebug/content/firefox/firebugFrame.xul" disablehistory="true"/>
    </vbox>
</vbox>

That's how Firebug puts itself into the browser window (and btw. using type="chrome").

Firebug has been forced to use <browser> since there were some font-zoom issues when using <iframe> Are there still any expected problems with using <browser> ?

Also, Firebug is importing several JSMs, mostly simple stuff but there is firebug-service.js module implementing Firebug's debugger API based on JSD. Is it possible to load such JSM into the content process as well?

Also, if I understand correctly, all overlays applied on the linked <browser> element will also run in the same process, correct? (I am thinking about Firebug extensions here)

Honza

Comment 27

[Rob Campbell [:rc] (:robcee)]

marking this as assigned since Olli's deep into it.

Honza, not sure I understand your question. You have overlays *inside* your browser element or containing the <browser>? If containing, then no, I don't think they'll be in the same process.

is there a good reason you can't move from <browser> to <iframe>? I was under the impression browsers were used when Firebug was created because iframes weren't really a viable option inside xul.

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Rob Campbell [:rc] (robcee) from comment #27)
> marking this as assigned since Olli's deep into it.
> 
> Honza, not sure I understand your question. You have overlays *inside* your
> browser element or containing the <browser>? If containing, then no, I don't
> think they'll be in the same process.
The overlays are inside the browser element.

> is there a good reason you can't move from <browser> to <iframe>? I was
> under the impression browsers were used when Firebug was created because
> iframes weren't really a viable option inside xul.

We had couple of problems with iframe in the past. One related to a security issue (bug 665369) and the type attribute of the iframe. The second was related to the text-zoom, since content of the iframe is XUL and it behaves differently from using <browser> element

These issues could be perhaps fixed, but it would be easier for Firebug to continue running in <browser>.

Honza

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I tried to apply the patch on fx-team repo (cloned now), but there are same failures. Should I use different repository?

Honza

$ patch -p1 --dry-run < linked_browsers_4.diff
patching file content/base/public/nsIFrameLoader.idl
patching file content/base/public/nsIFrameMessageManager.idl
patching file content/base/src/nsFrameLoader.cpp
Hunk #2 FAILED at 324.
Hunk #3 FAILED at 415.
Hunk #4 succeeded at 456 (offset -3 lines).
Hunk #5 FAILED at 768.
Hunk #7 FAILED at 943.
Hunk #8 FAILED at 1260.
Hunk #9 succeeded at 1340 (offset -6 lines).
Hunk #11 succeeded at 1816 (offset -6 lines).
Hunk #13 succeeded at 2073 (offset -6 lines).
Hunk #14 succeeded at 2195 with fuzz 1.
5 out of 14 hunks FAILED -- saving rejects to file content/base/src/nsFrameLoade
r.cpp.rej
patching file content/base/src/nsFrameLoader.h
Hunk #2 succeeded at 163 with fuzz 1.
Hunk #4 FAILED at 250.
1 out of 6 hunks FAILED -- saving rejects to file content/base/src/nsFrameLoader
.h.rej
patching file content/base/src/nsFrameMessageManager.cpp
patching file content/base/src/nsGkAtomList.h
Hunk #1 succeeded at 595 (offset 3 lines).
patching file content/base/src/nsInProcessTabChildGlobal.cpp
Hunk #2 FAILED at 391.
1 out of 2 hunks FAILED -- saving rejects to file content/base/src/nsInProcessTa
bChildGlobal.cpp.rej
patching file content/base/src/nsInProcessTabChildGlobal.h
patching file content/base/test/chrome/Makefile.in
patching file content/base/test/chrome/file_bug669698.xul
patching file content/base/test/chrome/test_bug669698.xul
patching file dom/ipc/PBrowser.ipdl
patching file dom/ipc/TabChild.cpp
Hunk #1 FAILED at 823.
1 out of 2 hunks FAILED -- saving rejects to file dom/ipc/TabChild.cpp.rej
patching file dom/ipc/TabChild.h
patching file layout/generic/nsSubDocumentFrame.h