Closed Bug 1165309 Opened 5 years ago Closed 4 years ago

[e10s] [dom.ipc.processCount>1] View source is blank

Categories

(Toolkit :: View Source, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
e10s later ---
firefox40 --- unaffected
firefox51 --- fixed

People

(Reporter: jonathan, Assigned: mrbkap)

References

Details

(Keywords: regression, Whiteboard: [e10s-multi:M1])

Attachments

(1 file, 3 obsolete files)

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

("view-source:"+url works fine.)
Blocks: e10s
Component: General → View Source
Product: Firefox → Toolkit
This seems to be a regression. Adding a tracking flag for firefox41.
Note that dom.ipc.processCount > 1 is not currently supported as a configuration.
Removed tracking for 41 (no official support.)
Aurora 41.0a2 not affected by default since now using tab. Bug 1067325
This seems to affect both window and tab forms of view source when processCount > 1.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.)
In tab was likely regressed by: Bug 1201535 (as pointed out in bug 1213693.)
(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).
Is here some ETA? The dom.ipc.processCount>1 is actually very useful, and this bug is the only noticeable drawback.
(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.
Duplicate of this bug: 1245040
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.
Duplicate of this bug: 1245040
: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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
(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/
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?
(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.
(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
Duplicate of this bug: 1261325
Attached patch bug1165309.patch (obsolete) — Splinter Review
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.
Attachment #8742767 - Flags: feedback?(mconley)
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?
Attachment #8742767 - Flags: feedback?(mconley) → feedback-
Flags: needinfo?(gkrizsanits)
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.
Depends on: 1251964
Flags: needinfo?(gkrizsanits)
(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?
Flags: needinfo?(gkrizsanits)
(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.
Flags: needinfo?(gkrizsanits)
(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.
Flags: needinfo?(gkrizsanits)
(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.
Flags: needinfo?(gkrizsanits)
Ah, gotcha, okay. Thanks gabor!
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.
(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.
Whiteboard: [e10s-multi:M1]
Duplicate of this bug: 1287637
Assignee: nobody → mrbkap
Duplicate of this bug: 1294177
Attached patch Attempt 2 (obsolete) — Splinter Review
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.
Attachment #8781320 - Flags: feedback?(mconley)
Attachment #8742767 - Attachment is obsolete: true
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.
Attachment #8781320 - Flags: feedback?(mconley) → feedback+
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8781766 - Flags: review?(mconley)
Attachment #8781320 - Attachment is obsolete: true
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>
Attachment #8781766 - Flags: review?(mconley) → review-
Attached patch Patch v2Splinter Review
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
Attachment #8782173 - Flags: review?(mconley)
Attachment #8781766 - Attachment is obsolete: true
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!
Attachment #8782173 - Flags: review?(mconley) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/02ededf61cbe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1310426
You need to log in before you can comment on or make changes to this bug.