943704 - importScripts() in child worker fails on B2G

Status: RESOLVED INCOMPLETE

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I was able to track down the importScripts() error in a child worker from Bug 925437 to HttpChannelChild::AsyncOpen. On B2G, it fails on the MissingRequiredTabChild() check [1].

Attached test case passes on Firefox desktop (Linux), but fails on B2G emulator running with Gecko compiled from mozilla-central.

[1]: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1059

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Test case

Test case.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8338979 [details] [diff] [review]
Test case

Can be applied to m-c

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This is going to need Necko help. Adding some folks, please add others?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

This presumably occurs because ChannelFromScriptURL() in dom/workers/ScriptLoader.cpp passes callbacks=nullptr to NS_NewChannel().

Comment 5

[Josh Matthews [:jdm]]

We should also be delegating to the loadgroup's callbacks if no callbacks are present. Are we associating a non-null document with this load?

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Josh Matthews [:jdm] from comment #5)
> We should also be delegating to the loadgroup's callbacks if no callbacks
> are present. Are we associating a non-null document with this load?

I'll trace this and update.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Nikhil Marathe [:nsm] (use needinfo?) from comment #6)
> (In reply to Josh Matthews [:jdm] from comment #5)
> > We should also be delegating to the loadgroup's callbacks if no callbacks
> > are present. Are we associating a non-null document with this load?
> 
> I'll trace this and update.

Yes, ChannelFromScriptURLWorkerThread() uses a main thread runnable to get a 'good' channel even for child workers. Since this works on desktop, I'm inclined to think the problem may be with HttpChannelChild doing something different than HttpChannel?

Comment 8

[Josh Matthews [:jdm]]

(In reply to Nikhil Marathe [:nsm] (use needinfo?) from comment #7)
> (In reply to Nikhil Marathe [:nsm] (use needinfo?) from comment #6)
> > (In reply to Josh Matthews [:jdm] from comment #5)
> > > We should also be delegating to the loadgroup's callbacks if no callbacks
> > > are present. Are we associating a non-null document with this load?
> > 
> > I'll trace this and update.
> 
> Yes, ChannelFromScriptURLWorkerThread() uses a main thread runnable to get a
> 'good' channel even for child workers. Since this works on desktop, I'm
> inclined to think the problem may be with HttpChannelChild doing something
> different than HttpChannel?

I don't really understand this comment. The important information here is whether there is a loadgroup present for the channel that is created by ScriptLoader. Comparing whether it works on desktop or not doesn't really make sense (assuming that OOP is disabled on desktop), since the tab child missing check only occurs when OOP is enabled.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

<nsm> ok, so here's what is going wrong - could you bring up http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#307
<nsm> jdm: ^
<nsm> the key is line 346, call to ForgetWorkerChannel()
<nsm> in the case of a toplevel worker, the worker holds a reference to a document. so when importScripts() is called, although the worker has forgotten its channel, it can use the doc and loadgroup to construct a valid channel that has that MissingRequiredTab check succeed
<nsm> in the case of a child worker, there is no reference to a document. the worker forgets it when its own script is loaded. When importScripts() is called, it has no channel, no parent doc, no load group. So the resulting channel isn't satisfying the check

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Ben, the ForgetWorkerChannel() was introduced in the SharedWorker patch. Is there a reason that the channel should be forgotten. In that case child workers should have an associated document in GetLoadInfo(), but there are currently main thread assertions preventing parts of it, and I'm not sure how good it would be to just set loadInfo.mWindow = aParent->GetWindow().

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

jduell, anything Necko could tell us?

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #10)
> Ben, the ForgetWorkerChannel() was introduced in the SharedWorker patch. Is
> there a reason that the channel should be forgotten. In that case child
> workers should have an associated document in GetLoadInfo(), but there are
> currently main thread assertions preventing parts of it, and I'm not sure
> how good it would be to just set loadInfo.mWindow = aParent->GetWindow().

It is fine to foget the channel, since the channel is only used to load the worker script.

Comment 13

[Jason Duell]

It looks like the issue here isn't necko per se, but that necko isn't being given the info it needs to do security checks (LoadInfo, from either channel or loadgroup callbacks).