569227 - Join HttpChannelChild with its TabChild

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: preview 1

TabChild::RecvloadURI() calls mWebNav->LoadURI.  A channel created by it doesn't know anything about the TabChild that invoked its load.

This is the missing piece to join HttpChannelParent with the correct TabParent on the chrome process.  This is needed mainly for the auth prompt code.


The patch is a preview of how I want to get the TabChild instance.  As TabChild sets itself as ContainerWindow of its mWebNav, i.e. it is set as WebBrowserChrome on the nsWebNav::mDocShellTreeOwner (sets mOwnerRequestor on it), and nsWebNav::mDocShell->mTreeOwner is set to nsWebNav::mDocShellTreeOwner, TabChild is then accessible through GetInterface(nsITabChild) on DocShell's (channel callbacks) TreeOwner.  nsDocShellTreeOwner::GetInterface delegates to mOwnerRequestor (TabChild) GetInterface.  This means that TabChild::GetInterface always has to return it self in response to GI(nsITabChild) (see also bug 537429).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Do we only need this for document channels?  For non-document channels the channel callbacks aren't going to be the docshell.

Comment 2

[Honza Bambas (:mayhemer)]

At the moment, I can say that this joining should be made for any request that may invoke authentication prompt.  And I presume there will be other reasons to have it.

Then, I should use GetCallback to also check loadgourp's callbacks that should be docshell (AFAIK) ?

Comment 3

[Boris Zbarsky [:bzbarsky]]

That would actually not do the right thing either.  You don't actually need a docshell, do you?  You need a nsITabChild.  So just change nsDocShell::GetInterface to hand one out (if it has one, of course), and change nsDocument::LoadgroupCallbacks to expose nsITabChild (since I would assume loads that use those callbacks can pose auth prompts).  Then you can just use NS_QueryNotificationCallbacks here and be done, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Or GetCallback, of course.

Comment 5

[Honza Bambas (:mayhemer)]

Changing nsDocShell::GetInterface will work only for document loads, right?

According to [1] the loadgroup callbacks are set to a proxy to the docshell, so apply the path I apply to channel's callbacks to loadgroup's callbacks should work, or no?  True is that it won't work for img requests...

However, it would be great to expose the interface directly.  Internally the path will be the same: docShell->mTreeOwner->mOwnerRequestor, thought.


[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#757

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Changing nsDocShell::GetInterface will work only for document loads, right?

Not if you use GetCallback (since that will also examine the loadgroup callbacks).

> According to [1] the loadgroup callbacks are set to a proxy to the docshell,

Right.

> Internally the path will be the same: docShell->mTreeOwner->mOwnerRequestor,
> thought.

For docshell, yes.  For the nsDocument::LoadgroupCallbacks case, not quite.  The point is that the http channel code shouldn't assume a particular path.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1

Boris, thanks for your comments.  Verified for images as well.  I cannot go the way of SHIM for nsITabChild in nsDocument resource code, I need to static_cast the instance I get.  Not sure how much security problem this could be.

(based on top of patch for security UI)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Right; not using TRY_SHIM in this case is fine.

Comment 9

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

Comment on attachment 448610 [details] [diff] [review]
v1


>-NS_IMPL_ISUPPORTS3(TabParent, nsIWebProgress, nsISecureBrowserUI, nsISSLStatusProvider)
>+NS_IMPL_ISUPPORTS4(TabParent, 
>+                   nsIWebProgress, 
>+                   nsISecureBrowserUI, 
>+                   nsISSLStatusProvider,
>+                   nsITabParent)
Perhpas nsITabParent could be the first interface, since that is the first interface
the class inherits.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to comment #9)
> Perhpas nsITabParent could be the first interface...

I sort interfaces by frequency of usage.  It's a little perf benefit when often used are first; but I have no numbers.  Will put it as the first one.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v2 [Checkin e10s comment 15]

Updated, merged.

Comment 12

[Jason Duell]

So it sounds like we're going to want to provide an accessor method for the TabParent, so that necko clients can get to it.  Dave Dahl needs this for Firebug.

Honza, can you add?

Comment 13

[Honza Bambas (:mayhemer)]

And where from the accessor should be reachable?  To what interface to add it?

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to comment #12)
> So it sounds like we're going to want to provide an accessor method for the
> TabParent...

This belongs to a different bug.

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 450753 [details] [diff] [review]
v2 [Checkin e10s comment 15]

http://hg.mozilla.org/projects/electrolysis/rev/f80302dc4c6c

Comment 16

[David Dahl :ddahl]

(In reply to comment #15)
> (From update of attachment 450753 [details] [diff] [review])
> http://hg.mozilla.org/projects/electrolysis/rev/f80302dc4c6c

Looks like this patch is e10s branch only - I assume then it will not work on mozilla-central, correct?

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to comment #16)
> I assume then it will not work on
> mozilla-central, correct?

Exactly.  It doesn't make any sense for m-c, until we merge it with the e10s tree.

Comment 18

[David Dahl :ddahl]

(In reply to comment #17)
> (In reply to comment #16)
> > I assume then it will not work on
> > mozilla-central, correct?
> 
> Exactly.  It doesn't make any sense for m-c, until we merge it with the e10s
> tree.

Does anyone know when this might be activated in a Firefox 4 beta?

Comment 19

[Josh Matthews [:jdm]]

Well, the code's all present in m-c, but there are no content processes in FF4.

Comment 20

[David Dahl :ddahl]

(In reply to comment #19)
> Well, the code's all present in m-c, but there are no content processes in FF4.

Thanks. Will there be? What bug is the content processes bug?

Comment 21

[Josh Matthews [:jdm]]

There will be no content processes in FF4; they will come at some later time (FF5, maybe?).  All content process work is happening in Fennec right now.