Closed Bug 569227 Opened 10 years ago Closed 10 years ago

Join HttpChannelChild with its TabChild

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch preview 1 (obsolete) — Splinter Review
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).
Attachment #448377 - Flags: feedback?(benjamin)
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Do we only need this for document channels?  For non-document channels the channel callbacks aren't going to be the docshell.
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) ?
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?
Or GetCallback, of course.
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
> 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.
Attachment #448377 - Flags: feedback?(benjamin)
Attached patch v1 (obsolete) — Splinter Review
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)
Attachment #448377 - Attachment is obsolete: true
Attachment #448610 - Flags: review?(benjamin)
Right; not using TRY_SHIM in this case is fine.
Blocks: 537782
Attachment #448610 - Flags: review?(benjamin) → review?(Olli.Pettay)
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.
Attachment #448610 - Flags: review?(Olli.Pettay) → review+
(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.
Updated, merged.
Attachment #448610 - Attachment is obsolete: true
Attachment #450753 - Flags: review+
Blocks: 536301
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?
And where from the accessor should be reachable?  To what interface to add it?
(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.
Blocks: 572631
Comment on attachment 450753 [details] [diff] [review]
v2 [Checkin e10s comment 15]

http://hg.mozilla.org/projects/electrolysis/rev/f80302dc4c6c
Attachment #450753 - Attachment description: v2 → v2 [Checkin e10s comment 15]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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?
(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.
(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?
Well, the code's all present in m-c, but there are no content processes in FF4.
(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?
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.
You need to log in before you can comment on or make changes to this bug.