Closed Bug 1212718 Opened 5 years ago Closed 5 years ago

chrome.webRequest.onCompleted never fires

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mao, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webRequest])

Attachments

(4 files)

Attached file 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).
Whiteboard: [webRequest]
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.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(amckay)
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).
Attached patch patchSplinter Review
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.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(amckay)
Attachment #8672844 - Flags: review?(dtownsend)
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).
Attached patch sendHeaders testSplinter Review
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.
Attachment #8672845 - Flags: review?(dtownsend)
Attached patch onCompleted testSplinter Review
Here's a test for the onCompleted issue. I had to refactor the test a bit.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8672846 - Flags: review?(dtownsend)
Attachment #8672844 - Flags: review?(dtownsend) → review+
Attachment #8672845 - Flags: review?(dtownsend) → review+
Attachment #8672846 - Flags: review?(dtownsend) → review+
Last two commits were actually for bug 1209983.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.