1022281 - [Browser API][Mochitest] test_browserElement_Download exposes failure to suspend and resume the HTTP channel during divert

Status: RESOLVED INCOMPLETE

(Core :: DOM: Device Interfaces, defect)

Description

[Aus Lacroix [:aus]]

On a device and in B2G desktop under normal operation, the download method works without any problem (I haven't tested Firefox, but, it should be the same as B2G Desktop).

When running in the testing framework though, it fails in a really weird way!

BrowserElementParent.jsm - fireEventFromMsg: titlechange, ""
BrowserElementParent.jsm - fireEventFromMsg: loadend, {"backgroundColor":"transparent","msg_name":"loadend"}
BrowserElementParent.jsm - _options = ({filename:"test.bin"})
BrowserElementParent.jsm - Setting HTTP referrer = [xpconnect wrapped nsIURI @ 0x45231d60 (native @ 0x4025b2c4)]
BrowserElementParent.jsm - DownloadListener Constructor
BrowserElementParent.jsm - fireEventFromMsg: firstpaint, {"msg_name":"firstpaint"}
BrowserElementParent.jsm - fireEventFromMsg: documentfirstpaint, {"msg_name":"documentfirstpaint"}
[Child 355] WARNING: Transparent content with displayports can be expensive.: file /home/aus/Projects/mozilla-central/layout/base/nsDisplayList.cpp, line 1357
BrowserElementParent.jsm - DownloadListener - onStartRequest
[Parent 297] WARNING: NS_ENSURE_TRUE(mIsPending) failed: file /home/aus/Projects/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp, line 4401
[Parent 297] WARNING: NS_ENSURE_TRUE(mIsPending) failed: file /home/aus/Projects/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp, line 4401
System JS : ERROR (null):0 - uncaught exception: 2147746065
[Parent 297] WARNING: NS_ENSURE_TRUE(mSuspendCount > 0) failed: file /home/aus/Projects/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp, line 4418
[Parent 297] WARNING: The dialog should nullify the dialog progress listener: file /home/aus/Projects/mozilla-central/uriloader/exthandler/nsExternalHelperAppService.cpp, line 2069
[Parent 297] WARNING: A control runnable was posted to a worker that is already shutting down!: file /home/aus/Projects/mozilla-central/dom/workers/WorkerPrivate.cpp, line 2243
[Parent 297] WARNING: A control runnable was posted to a worker that is already shutting down!: file /home/aus/Projects/mozilla-central/dom/workers/WorkerPrivate.cpp, line 2243
[Parent 297] WARNING: A control runnable was posted to a worker that is already shutting down!: file /home/aus/Projects/mozilla-central/dom/workers/WorkerPrivate.cpp, line 2243
[Parent 297] WARNING: A control runnable was posted to a worker that is already shutting down!: file /home/aus/Projects/mozilla-central/dom/workers/WorkerPrivate.cpp, line 2243

I'm digging into this and it should be resolved during stabilization.

Comment 1

[Aus Lacroix [:aus]]

Steve, would you have some time to help me dig into this? I'll get some better logs up soon. On the surface it looks like when we are running in Single Process mode in the test harness we fail to suspend the channel which causes the hand off on divert to fail to resume the channel later.

However, I'm not sure where all this magic happens. :)

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Aus, no problem. I'll try to find some time later this afternoon.

In the meantime, one thing: ChannelDiverter shouldn't be suspending anything in single process mode. It's only for e10s. So, maybe I'm missing something - can you send me info on how the download method is integrated with Necko, what it's trying to do etc? Then I can tell you about the magic :)

Comment 3

[Aus Lacroix [:aus]]

OK, that's what I thought. It should be a no-op. :)

Here's where the download is started: https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.jsm#624

We never make it to DoContent in the DownloadListener when running this test: https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/browserElement_Download.js

The test itself always sets these prefs here: https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/browserElementTestHelpers.js#246

Comment 4

[Aus Lacroix [:aus]]

Well, it definitely looks like I'm running into the original problem I had when I had the implementation live in BrowserElementChildPreload. 

The AsyncOpen is happening in the wrong process which is causing my StreamListener to get released. Which in turn fails to call OnStopRequest on the external app handler instance that is managing the download.

This is definitely in part because the iframe in this scenario is not hosted in the same manner as it would as tested in Firefox OS.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Aus - sorry, but I got caught up in something else this afternoon :( I'll find time to take a look tomorrow.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Hey Aus,
Looking at this now. Some questions/thoughts:

-- Most likely not the cause of the error you're seeing, but in browserElement_Download.js in loadend() it looks like the callbacks for req are set after the download is started. Just wondering if that's a possible issue since this is async. Wondering if there are threads at play underneath that might mess with that.

-- You mention that doContent is never called; it's on onStartRequest - is that def called?

-- Due to the values set in those prefs, we're probably not in single process mode here. It looks to me like frames are created OOP. This would explain why ChannelDiverter is doing anything here at all :)

-- But that seems weird because it's BrowserElementParent._download. So, why is that being called and not BrowserElementChild's download function? (I assume there is a BrowserElementChild.

So, those last two seems to concur with your comment #4.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Steve Workman [:sworkman] from comment #6)
> -- But that seems weird because it's BrowserElementParent._download. So, why
> is that being called and not BrowserElementChild's download function? (I
> assume there is a BrowserElementChild.
> 
> So, those last two seems to concur with your comment #4.

... assuming, of course, that BrowserElementParent is being called in the child process :)

Comment 8

[Aus Lacroix [:aus]]

(In reply to Steve Workman [:sworkman] from comment #6)
> Hey Aus,
> Looking at this now. Some questions/thoughts:
> 
> -- Most likely not the cause of the error you're seeing, but in
> browserElement_Download.js in loadend() it looks like the callbacks for req
> are set after the download is started. Just wondering if that's a possible
> issue since this is async. Wondering if there are threads at play underneath
> that might mess with that.

Thankfully the way Requests work onsuccess/onerror will get called when a function is assigned to them and the operation has already completed.

> 
> -- You mention that doContent is never called; it's on onStartRequest - is
> that def called?

Yes, that's definitely called. I end up with a completed download on the sdcard as expected but it's still a .part file because onStopRequest never gets called.

> 
> -- Due to the values set in those prefs, we're probably not in single
> process mode here. It looks to me like frames are created OOP. This would
> explain why ChannelDiverter is doing anything here at all :)

Yep. :)
 
> -- But that seems weird because it's BrowserElementParent._download. So, why
> is that being called and not BrowserElementChild's download function? (I
> assume there is a BrowserElementChild.
> 
> So, those last two seems to concur with your comment #4.

Yep. It would seem that to ensure that this works under all working conditions I will have to create Parent/Child StreamListener implementation. :(

In the meantime, I'm attempting to write a mochitest that tests the download method as it is used in FFOS today.

Comment 9

[Aus Lacroix [:aus]]

So, there are fundamental differences in how the test container runs compared to the system app and it would require quite a bit of work to make this test run as a mochitest.

I'm going to instead write a unit test for the functionality in the browser app which in turn uses this api. This will ensure we catch any breakage for the time being.

Comment 10

[Aus Lacroix [:aus]]

Alright, after much debate and attempts to write tests for this I'm basically stuck. The test infrastructure doesn't seem to support the type of process model that I need to write them.

I'm going to put this on ice for now.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Cleaning up Device Interfaces component, and mass-marking old FxOS bugs as incomplete.

If any of these bugs are still valid, please let me know.