Closed Bug 1603666 Opened 4 years ago Closed 4 years ago

Attempts to display blocked resources with no blocking reason leak

Categories

(DevTools :: Netmonitor, defect)

defect
Not set
normal

Tracking

(firefox73 fixed)

RESOLVED FIXED
Firefox 73
Tracking Status
firefox73 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Consider this testcase:

<video>
  <track src="http://this-is-not-the-site-I-am-hosted-on-I-am-sure" default>
</video>

This testcase leaks when loaded while the network tab in devtools is open. This is easy to see by setting XPCOM_MEM_BLOAT_LOG to some filename, starting a debug browser, opening the network panel, loading the testcase, then quitting.

This leak is what caused bug 1602090 to get backed out, since all cases that lead to the error message being surfaced there also lead to this leak.

The leak is caused by the fact that in this case the channel is blocked without setting a requestBlockingReason on the loadinfo. Then we end up in _httpFailedOpening with a topic of "http-on-failed-opening-request". That calls _httpResponseExaminer, which calls _createNetworkEvent. The blockedReason passed in at this point is 0, which does NOT mean "not blocked"; it just means "whoever blocked this did not provide a reason".

Then createNetworkEvent does:

    if (!event.blockedReason) {
      this._setupResponseListener(httpActivity, fromCache);
    }

which ends up throwing on this line:

    const originalListener = channel.setNewListener(tee);

because at this point the channel is not in a state where you can mess with it's listener: it's already canceled.

Unfortunately, before getting to the exception point _setupResponseListener creates a cycle that includes the channel. In particular, the NetworkResponseListener is in such a cycle, and the channel has already dropped its notification callbacks and whatnot when it errored out (the OnFailedOpeningRequest call is the last thing in HttpChannelChild::AsyncOpen), so nothing is going to break the cycle after this point.

It looks like this code was added in bug 1151368, so needinfoing the people who might know something about it...

Things we could consider doing:

  1. HttpChannelChild::AsyncOpen could re-clear members after dispatching the failure notification, or prevent them from being set after they got cleared earlier, to avoid leaks.

  2. The content security manager could set a blocking reason when it blocks due to failing SOP checks. That said, there are lots of things that can lead to blocking and I don't know that all the other ones set a reason...

  3. The devtools code could avoid losing track of the fact that this is a failed load. In particular, it could replace !blockedReason checks with blockedReason != undefined checks, since 0 is a perfectly valid value of blockedReason when blocking happened.

Flags: needinfo?(poirot.alex)
Flags: needinfo?(odvarko)
Flags: needinfo?(jryans)
Blocks: 1602090

I'm going to give option 3 a shot.

Assignee: nobody → bzbarsky

Not everything that blocks requests sets a reason other than BLOCKING_REASON_NONE.

Option 3 seems like a fine approach to me.

Blocks: 1603805
Blocks: 1603806
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb9911385c92
Network monitor should not lose track of whether the request was blocked.  r=ochameau,jryans
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
Flags: needinfo?(poirot.alex)
Flags: needinfo?(odvarko)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: