Jonathan Howard Reporter
	Description • 9 years ago

After bug 1025146 landed the window opens but shows no source.
Error printed in browser console.

("view-source:"+url works fine.)

	Loic
	Comment 1 • 9 years ago

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8b64c75b0b86&tochange=62d9b117c688

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 2 • 9 years ago

This seems to be a regression. Adding a tracking flag for firefox41.

	Mike Conley (:mconley) (:⚙️)
	Comment 3 • 9 years ago

Note that dom.ipc.processCount > 1 is not currently supported as a configuration.

	Jonathan Howard Reporter
	Comment 4 • 9 years ago

Removed tracking for 41 (no official support.)
Aurora 41.0a2 not affected by default since now using tab. Bug 1067325

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 7 • 9 years ago

This seems to affect both window and tab forms of view source when processCount > 1.

	Jonathan Howard Reporter
	Comment 8 • 9 years ago

Note: as of 43.0a2 regressed with default of open in tab. Requires clicking view page source on a page, (typing in address "view-source:"+url still works.)

	Jonathan Howard Reporter
	Comment 10 • 9 years ago

In tab was likely regressed by: Bug 1201535 (as pointed out in bug 1213693.)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 11 • 9 years ago

(In reply to Jonathan Howard from comment #10)
> In tab was likely regressed by: Bug 1201535 (as pointed out in bug 1213693.)

Yes, I can see how that would happen.  We'll need some way to open a new tab in the same content process as another, but without directly using the original tab's URL (to avoid requests).

	Eugene Savitsky
	Comment 13 • 9 years ago

Is here some ETA? The dom.ipc.processCount>1 is actually very useful, and this bug is the only noticeable drawback.

	Mike Conley (:mconley) (:⚙️)
	Comment 14 • 9 years ago

(In reply to Eugene Savitsky from comment #13)
> Is here some ETA? The dom.ipc.processCount>1 is actually very useful, and
> this bug is the only noticeable drawback.

We're tracking the work to support multiple content processes in bug 1207306, if you'd like to follow along.

	:Gijs (he/him)
	Comment 16 • 8 years ago

From the dupe:

I just tried to reproduce. When this happens this shows up in the error console:

TypeError: contentWindow is null viewSource-content.js:227:11


Which is this code:

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/toolkit/components/viewsource/content/viewSource-content.js#222-227

which presumably fails because the window isn't in the process that receives the message, if the frame script is inserted into multiple processes and all of them get the message.

---

I don't know if we can workaround by just not requiring the contentWindow and falling back to some other behaviour, or if we can use some other trick to ensure view source always loads in the same content process as the original frame/page.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 18 • 8 years ago

:billm, is there any existing support for something like "load tab B in the same content process as this other tab A"?  

I believe that's at least part of what we'd need here in view source, since we access the page descriptor of the original page to avoid loading from the network, and so we need to be in the same process as the original tab.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 19 • 8 years ago

The only way this happens now is if a content window calls window.open(...). In that case, we go through this path:

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#784
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#875
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#5414
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#5529
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4833
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4734

It's possible that a frame script in the child could call window.open and cause a new tab to open. Then the view source code could assume control of that tab on the parent side. Maybe you could match them up using the outer window ID or something.

Note that we may end up adding some better platform mechanism for doing this for WebExtensions. But this is currently the only way to do it.

	avada
	Comment 20 • 8 years ago

(In reply to Eugene Savitsky from comment #13)
> Is here some ETA? The dom.ipc.processCount>1 is actually very useful, and
> this bug is the only noticeable drawback.

For the time being you can work around with the "View Source Choice" addon: https://addons.mozilla.org/addon/view-source-choice/

	Julien Wajsberg [:julienw]
	Comment 23 • 8 years ago

I have also an issue with the print preview, do you think it could be the same root cause or should I file a separate bug?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 24 • 8 years ago

(In reply to Julien Wajsberg [:julienw] from comment #23)
> I have also an issue with the print preview, do you think it could be the
> same root cause or should I file a separate bug?

It sounds different to me, I'd say please file separately.

	Jonathan Howard Reporter
	Comment 25 • 8 years ago

(In reply to Julien Wajsberg [:julienw] from comment #23)
> I have also an issue with the print preview, do you think it could be the
> same root cause or should I file a separate bug?
Essentially same and is bug 1142013

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 27 • 8 years ago

This does what's being suggested in comment 19. Unfortunately that comment only works for view source in tab -- I don't know how to move the viewSource.xul loaded by openDialog into content process.

The tab opened by window.open happen to be also relatedToCurrent=true and inBackground=false. If that has to change in the future content would need some way to do so.

	Mike Conley (:mconley) (:⚙️)
	Comment 28 • 8 years ago

Comment on attachment 8742767 [details] [diff] [review]
bug1165309.patch

Review of attachment 8742767 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know how I feel about this.

I feel like we should just make it simpler to request new nsITabParent's based on some pre-existing nsITabParent.

Especially as we start moving towards a multi-content-process model, I suspect doing so will get important. Doing the message down to the child to send a message to the parent to open a window... seems fragile.

Hey gabor - do you know if there are any current thoughts on exposing any APIs in the parent for creating new remote browsers sharing the same content process as pre-existing ones?

	Gabor Krizsanits (INACTIVE)
	Comment 29 • 8 years ago

This sounds like bug 1251964. I think this should be fixed in window watcher. When SendBrowserFrameOpenWindow is called from ContentChild::ProvideWindowCommon, on parent side it should be easy to identify the child process. We just have to ensure that this call is from window.open and then we always want to create the new window in the same process.

	Mike Conley (:mconley) (:⚙️)
	Comment 30 • 8 years ago

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #29)
> This sounds like bug 1251964. I think this should be fixed in window
> watcher. When SendBrowserFrameOpenWindow is called from
> ContentChild::ProvideWindowCommon, on parent side it should be easy to
> identify the child process. We just have to ensure that this call is from
> window.open and then we always want to create the new window in the same
> process.

Let me just make sure we're clear:

For View Source, for example, where the parent initiates a tab or window being opened, and it's vitally important that the newly opened tab or window remote <xul:browser> runs in the same process as the one originally selected. Print preview is similar where the parent initiates the opening of a new <xul:browser> that is being associated with the currently selected one, and it's important that the browsers share content processes.

So, to be clear, for multiple content processes, the plan we want to go with for the above cases is to send a message to the child causing it to window.open, and then having the parent capture the newly opened window or tab?

	Gabor Krizsanits (INACTIVE)
	Comment 31 • 8 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> So, to be clear, for multiple content processes, the plan we want to go with
> for the above cases is to send a message to the child causing it to
> window.open, and then having the parent capture the newly opened window or
> tab?

Sorry, you're right I misunderstood things a bit. So window.open already does the right thing (bug 1251964 should be closed as invalid then I just really want to take some time to check it more thoroughly and I don't have the time for it right now). I thought we still have to do some work there and wanted to reuse that API here. But based on ContentParent::RecvCreateWindow there is already an option for specifying the process in the window watcher it's just not exposed to JS yet. Since we have multiple use cases where this would be helpful I think we should expose this option to JS.

	Mike Conley (:mconley) (:⚙️)
	Comment 32 • 8 years ago

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #31)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> > So, to be clear, for multiple content processes, the plan we want to go with
> > for the above cases is to send a message to the child causing it to
> > window.open, and then having the parent capture the newly opened window or
> > tab?
> 
> Sorry, you're right I misunderstood things a bit. So window.open already
> does the right thing (bug 1251964 should be closed as invalid then I just
> really want to take some time to check it more thoroughly and I don't have
> the time for it right now). I thought we still have to do some work there
> and wanted to reuse that API here. But based on
> ContentParent::RecvCreateWindow there is already an option for specifying
> the process in the window watcher it's just not exposed to JS yet. Since we
> have multiple use cases where this would be helpful I think we should expose
> this option to JS.

Okay, cool - one final question:

Are we certain that we want to send a message to the child to initiate the window / tab opening? Is it worth investigating an API that allows the parent to take some remote <xul:browser>, or frameloader, or nsITabParent, and produce another <xul:browser> that is guaranteed to be running in the same content process?

The reason I ask this is because I'm worried about code like what we have in the patch here - where, while we're waiting for the content process to send us a window, we add an event listener for the next opened window or tab... that seems like it might be error prone if, for example, the content process is blocked and the user opens a new window or tab while they're waiting, for example.

	Gabor Krizsanits (INACTIVE)
	Comment 33 • 8 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #32)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #31)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> > > So, to be clear, for multiple content processes, the plan we want to go with
> > > for the above cases is to send a message to the child causing it to
> > > window.open, and then having the parent capture the newly opened window or
> > > tab?
> > 
> > Sorry, you're right I misunderstood things a bit. So window.open already
> > does the right thing (bug 1251964 should be closed as invalid then I just
> > really want to take some time to check it more thoroughly and I don't have
> > the time for it right now). I thought we still have to do some work there
> > and wanted to reuse that API here. But based on
> > ContentParent::RecvCreateWindow there is already an option for specifying
> > the process in the window watcher it's just not exposed to JS yet. Since we
> > have multiple use cases where this would be helpful I think we should expose
> > this option to JS.
> 
> Okay, cool - one final question:
> 
> Are we certain that we want to send a message to the child to initiate the
> window / tab opening? Is it worth investigating an API that allows the
> parent to take some remote <xul:browser>, or frameloader, or nsITabParent,
> and produce another <xul:browser> that is guaranteed to be running in the
> same content process?
> 
> The reason I ask this is because I'm worried about code like what we have in
> the patch here - where, while we're waiting for the content process to send
> us a window, we add an event listener for the next opened window or tab...
> that seems like it might be error prone if, for example, the content process
> is blocked and the user opens a new window or tab while they're waiting, for
> example.

Sorry I think I was not clear in my previous comment. No we do not want to send a message to the child to initiate the opening. When I say expose the window watcher method to JS I mean parent side JS. The current C++ version of the API uses tabParent to identify the child process if I get this code right: http://hg.mozilla.org/mozilla-central/annotate/cfc7ebe59293/dom/ipc/ContentParent.cpp#l5559. I want to expose a similar method to JS.

	Mike Conley (:mconley) (:⚙️)
	Comment 34 • 8 years ago

Ah, gotcha, okay. Thanks gabor!

	Mike Conley (:mconley) (:⚙️)
	Comment 35 • 8 years ago

Okay timdream, so here's where we're at:

Your patch solves the problem in the short term, but I think it's kind of a brittle solution.

I think the long-term solution is to expose an API in the parent process that allows remote <xul:browser>'s to be run in the same content process as some other given remote <xul:browser>. We would then use that API when creating the remote <xul:browser> for the view source tab or window.

I've filed bug 1267653 to create such an API.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 36 • 8 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #35)
> Okay timdream, so here's where we're at:
> 
> Your patch solves the problem in the short term, but I think it's kind of a
> brittle solution.
> 

I agree with this once I wrote the patch; I uploaded the patch just for record keeping and to prove a point. Glad that initiated the discussion.

> I think the long-term solution is to expose an API in the parent process
> that allows remote <xul:browser>'s to be run in the same content process as
> some other given remote <xul:browser>. We would then use that API when
> creating the remote <xul:browser> for the view source tab or window.
> 
> I've filed bug 1267653 to create such an API.

I have not yet look into the code -- if it's not extremely hard I can try to pick it up.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 40 • 8 years ago

Getting information from the browser to its enclosing frameloader is one of the hardest things I think I've had to do. Especially, in this case, because a string isn't the right type and I really didn't want to have to hard code a PID or something like that.

This was the most obvious way to get the information through. Mike, is there a better method? Am I missing a downside to doing this? Ideas welcome.

	Mike Conley (:mconley) (:⚙️)
	Comment 41 • 8 years ago

Comment on attachment 8781320 [details] [diff] [review]
Attempt 2

Review of attachment 8781320 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is a fine approach, but I have some suggestions. Thanks mrbkap!

::: dom/ipc/nsIBrowser.idl
@@ +3,5 @@
> +interface nsIDOMElement;
> +interface nsIFrameLoader;
> +
> +[scriptable, uuid(14e5a0cb-e223-4202-95e8-fe53275193ea)]
> +interface nsIBrowser : nsISupports

I wonder if it makes sense to add a function to get at the relatedBrowser to nsIXULBrowserWindow instead, and avoid adding a new interface.

The nsIXULBrowserWindow interface is implemented per window, and is defined in browser/base/content/browser.js as XULBrowserWindow. You could, perhaps, add a method there that calls into gBrowser to get the related browser.

Heck, we could even make it easier on the caller, and have it return the related nsITabParent if one exists.

::: toolkit/content/widgets/browser.xml
@@ +232,5 @@
>        <property name="preferences"
>                  onget="return this.mPrefs.QueryInterface(Components.interfaces.nsIPrefService);"
>                  readonly="true"/>
>  
> +      <field name="relatedBrowser">null</field>

I wonder if this should always be a weak reference so that we don't end up keeping browsers alive needlessly.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 42 • 8 years ago

This seems to work. I refactored some code to get the nsIXULBrowserWindow out of an Element*. I took this opportunity to fix print preview (bug 1142013) at the same time. This also switches to using a weak reference. And returning an nsITabParent.

Mike, is my trick with && too tricky? I went back and forth over that a bit.

	Mike Conley (:mconley) (:⚙️)
	Comment 43 • 8 years ago

Comment on attachment 8781766 [details] [diff] [review]
Patch v1

Review of attachment 8781766 [details] [diff] [review]:
-----------------------------------------------------------------

Gah, my bad - see below. :/

::: browser/base/content/browser.js
@@ +4323,5 @@
>  
>      return true;
>    },
>  
> +  getRelatedTabParent: function(aBrowser) {

I'm so, so sorry to waffle on this, but I think I made a bad call before.

I was assuming that we'd only ever want to deal with the relationship between remote browsers in the same window. This is not the case if we're ending up with a new window - for example, if we're opening View Source in a window instead of a tab. The newly opened browser will not be part of anything that has a nsIXULBrowserWindow.

So I think it actually might be wiser to go back to your nsIBrowser interface from before. Really sorry about that, should have caught that edge case the first time around. :/

::: toolkit/content/widgets/browser.xml
@@ +236,5 @@
> +      <!--
> +        Weak reference to the related browser (see
> +        nsIXULBrowserWindow.getRelatedBrowser).
> +      -->
> +      <field name="_relatedBrowser">null</field>

What might be better is to just handle the weakref'ing here ourselves. For example:

<field name="_relatedBrowser">null</field>
<property name="relatedBrowser">
  <getter><![CDATA[
    return this._relatedBrowser && this._relatedBrowser.get();
  ]]></getter>

  <setter><![CDATA[
    this._relatedBrowser = Components.utils.getWeakReference(val);
  ]]></setter>
</property>

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 44 • 8 years ago

No worries about that. It was simple to re-incorporate everything. I've left a small change in TabParent::GetXULBrowserWindow since the QI was unnecessary.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80784af25c03

	Mike Conley (:mconley) (:⚙️)
	Comment 45 • 8 years ago

Comment on attachment 8782173 [details] [diff] [review]
Patch v2

Review of attachment 8782173 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! We might need a follow-up for the windowed View Source case though, since I think we're going to need to hook up the relatedBrowser there separately as well.

Great job!

	Mike Conley (:mconley) (:⚙️)
	Comment 46 • 8 years ago

Might also be a good idea to update the MDN documentation on <xul:browser> here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/browser

And maybe do a shout-out on dev-platform / firefox-dev about this mechanism for making sure that a browser opened in the parent can be opened in the same content process as another.

	Pulsebot
	Comment 47 • 8 years ago

Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02ededf61cbe
Make view source and print preview work in e10s-multi. r=mconley

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/02ededf61cbe