e10s HTTP: nsIDocShellTreeItem crashes

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: lusian, Unassigned)

Tracking

Other Branch
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
HttpChannelParent needs to implement nsIDocShellTreeItem.

Accessing http://bugzilla.mozilla.org kills the browser, too.
(Reporter)

Updated

8 years ago
Blocks: 516730
Er... why does something in _necko_ need to implement nsIDocshellTreeItem?
(Reporter)

Comment 2

8 years ago
I was testing Jason's patch in Bug 536292, and the browser died on the web sites.  Please advise.
Well, for starters not all HTTP channels have an associated nsIDocShellTreeItem.  Or more precisely, some HTTP channel callbacks don't hand one of those out from GetInterface for security reasons.
OK, so it sounds like our issue here is that something is trying to get a nsIDocShellTreeItem from HttpChannelParent via GetInterface().  Boris, does it makes sense to try to hand out a nsIDocShellTreeItem to something in chrome?  Jae, what's the code that's asking for the nsIDocShellTreeItem?  I'm guessing it may need to be changed.
> something is trying to get a nsIDocShellTreeItem from HttpChannelParent via
> GetInterface().

Quite likely, yes.  Can we hunt down that something?

> does it makes sense to try to hand out a nsIDocShellTreeItem to something in
> chrome? 

Only marginally.  Can we locate the caller and see what it really wants here?
(Reporter)

Comment 6

8 years ago
Created attachment 441711 [details]
stack trace

mozilla::net::HttpChannelParent::GetInterface Line 271 << dies here.
nsInterfaceRequestorAgg::GetInterface Line 62 + 0x24 bytes
nsHttpConnection::GetInterface Line 828 + 0x21 bytes
That's not very enlightening; I wonder who posts that event...

Comment 8

8 years ago
A fair bit of extension code expects to be able to get a docshell from HTTP requests so that it can decide whether to allow or prevent the request. I don't know whether product code makes similar assumptions. I suspect we are going to have to, at some point, be able to go from httpchannel -> PIFrameEmbedding or something like that, in order for security-UI to be implemented entirely on the chrome side.

I don't actually know what this request is, though... sounds like we need a longer stacktrace or better data.
> A fair bit of extension code expects to be able to get a docshell from HTTP
> requests so that it can decide whether to allow or prevent the request.

Yes, but we don't _have_ a docshell in the chrome process.  Do we just want to hand out a proxy?

And note that such code is broken and should be using nsILoadContext (which we should change as needed), not docshell.

Comment 10

8 years ago
Oh, I agree it's broken, or at least will be unavoidably broken when we have remote tabs. I just think we should design the replacement to actually meet our needs in-app as well as extensions.
Absolutely.  The idea of nsILoadContext was that we could modify it as needed to meet our needs as we did e10s...  Dunno that it succeeded at that.  ;)
(Reporter)

Comment 12

8 years ago
from nsHttpConnection.cpp:

// not called on the socket transport thread
NS_IMETHODIMP
nsHttpConnection::GetInterface(const nsIID &iid, void **result)
{
    // NOTE: This function is only called on the UI thread via sync proxy from
    //       the socket transport thread.  If that weren't the case, then we'd
    //       have to worry about the possibility of mTransaction going away
    //       part-way through this function call.  See CloseTransaction.
Summary: e10s HTTP: Implement nsIDocShellTreeItem → e10s HTTP: nsIDocShellTreeItem crashes
(Reporter)

Comment 13

8 years ago
from nsHttpConnection.cpp:

// not called on the socket transport thread
NS_IMETHODIMP
nsHttpConnection::GetInterface(const nsIID &iid, void **result)
{
    // NOTE: This function is only called on the UI thread via sync proxy from
    //       the socket transport thread.  If that weren't the case, then we'd
    //       have to worry about the possibility of mTransaction going away
    //       part-way through this function call.  See CloseTransaction.
Fixed as part of bug 536292.

http://hg.mozilla.org/projects/electrolysis/rev/11682e4090a9
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.