1212718 - chrome.webRequest.onCompleted never fires

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Giorgio Maone [:ma1]]

Attachment: oncompleted-0.1.xpi

See the attached sample XPI.
Furthermore, we should either implement onErrorOccurred or at least call onCompleted also when the request is aborted by an error (you can use a global nsIWebProgressListener to track that).

Comment 1

[Giorgio Maone [:ma1]]

OK, I've figured out what's going on.

At the time when onStopRequest is called on our channel listener, causing "onStop" and eventually "onCompleted" to be fired, HttpObserverManager.getLoadContext(channel) returns null, and therefore the "browser" variable is set to null:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#250

This causes the wrapper of the "onCompleted" event callback to return prematurely because the tab id could not be retrieved:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-webRequest.js#24

We need either to retrieve this info in some other way or to omit it from onCompleted's details.

Any ETA for a fix? 
I've got a demo in 2 days (which does not depend on the tab id being available in onCompleted, BTW) and this would really help.

Comment 2

[Giorgio Maone [:ma1]]

On a side note, outerWindowId is available at that time, so in the worst case (when browser is unavailable) we could always return tabId as a lazy property to be initialized on demand by iterating across the available browsers until we match the outer window id (maybe starting with the focused one as a tentative optimization).

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

The problem here, as Giorgio pointed out, is that the load context is null when we get the onStopRequest notification in e10s (in non-e10s it's still valid). That's presumably some leak prevention thing we do (none of the networking code is cycle collected).

This patch just keeps the load context alive by saving it in the StartStopListener. I was a bit concerned about leaks here, but the networking code seems to null out the listener for us later.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, this patch also fixes an unrelated bug I noticed where runChannelListener doesn't return true at the end (signalling that the load wasn't blocked).

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: sendHeaders test

This patch adds a test for the issue where we weren't returning true. The bug manifests in us not calling runChannelListener("afterModify") in some cases, which leads to onSendHeaders not firing.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: onCompleted test

Here's a test for the onCompleted issue. I had to refactor the test a bit.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d010e8c17cf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/db2151d8093a
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b05cd86727
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb924113e8c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/09f71d26b0e3

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Last two commits were actually for bug 1209983.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d010e8c17cf3
https://hg.mozilla.org/mozilla-central/rev/db2151d8093a
https://hg.mozilla.org/mozilla-central/rev/88b05cd86727
https://hg.mozilla.org/mozilla-central/rev/bb924113e8c6
https://hg.mozilla.org/mozilla-central/rev/09f71d26b0e3