Join HttpChannelChild with its TabChild

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Posted 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)

Updated

9 years ago
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.
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 5

9 years ago
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.

Updated

9 years ago
Attachment #448377 - Flags: feedback?(benjamin)
(Assignee)

Comment 7

9 years ago
Posted 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.
(Assignee)

Updated

9 years ago
Blocks: 537782

Updated

9 years ago
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+
(Assignee)

Comment 10

9 years ago
(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.
(Assignee)

Comment 11

9 years ago
Updated, merged.
Attachment #448610 - Attachment is obsolete: true
Attachment #450753 - Flags: review+
(Assignee)

Updated

9 years ago
Blocks: 536301

Comment 12

9 years ago
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?
(Assignee)

Comment 13

9 years ago
And where from the accessor should be reachable?  To what interface to add it?
(Assignee)

Comment 14

9 years ago
(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.

Updated

9 years ago
Blocks: 572631
(Assignee)

Comment 15

9 years ago
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]
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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?
(Assignee)

Comment 17

9 years ago
(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.