474704 - [@font-face] Leak nsCrossSiteListenerProxy and nsFontFaceLoader (loading 472237-1.html over file:)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Jesse Ruderman]

Attachment: trace-refcnt leak log

Steps to reproduce:
1. Run Firefox with trace-refcnt enabled.
2. Load layout/style/crashtests/472237-1.html using a file: URL.  (Note that in Tinderbox tests, it is loaded using an http: URL.)
3. Quit Firefox.

Result: trace-refcnt reports leaked nsCrossSiteListenerProxy, nsFontFaceLoader, etc.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do any of the content/base/test tests leak when run from file:?

Comment 2

[John Daggett (:jtd)]

Attachment: patch, v.0.1, explicitly break ref cycle on error

There appears to be a cycle between the fontloader/channel/streamloader objects, so explicitly break that when an error occurs.  Tested using induced failures in other places within the function to assure that no leaks occur in other error situations.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the cycle, I assume, is that channel holds a ref to the nsCrossSiteListenerProxy via notification callbacks, nsCrossSiteListenerProxy holds a ref to the streamloader via mOuterListener, streamloader holds a ref to the fontloader, and fontloader holds a ref to the channel?

Breaking the loop by dropping the channel ref is probably fine, since that's generally how necko lifetime is managed (you only hold on to channels that asyncopen succeeded on, and they make sure to drop their refs to everything when done).  But should the stream listener proxy perhaps also drop all its strong refs if UpdateChannel() fails in the constructor?  Jonas?

Comment 4

[Boris Zbarsky [:bzbarsky]]

In particular, if I'm correct there's no cycle to be broken if allocating the proxy fails, right?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 359477 [details] [diff] [review]
patch, v.0.1, explicitly break ref cycle on error

I'm just going to transfer this review request to bzbarsky, since he has a much better idea about the rules for using Necko than I do.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 359477 [details] [diff] [review]
patch, v.0.1, explicitly break ref cycle on error

r=bzbarsky if we make that function be DropChannel() (and always set to null) and if we don't call it if !listener, since there's no cycle in that case as far as I can tell.

Comment 7

[John Daggett (:jtd)]

Attachment: patch, v.0.2, changes based on bz review plus suggested change to nsCrossSiteListenerProxy

Jonas, does this look right to you?

Comment 8

[Boris Zbarsky [:bzbarsky]]

You probably want to drop the outer notification callbacks too, right?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 359726 [details] [diff] [review]
patch, v.0.2, changes based on bz review plus suggested change to nsCrossSiteListenerProxy

Looks good, but could you also add a test? Would be great to have tests with @font-face both pointing to invalid URLs, valid-but-non-http URLs and valid urls that return real fonts, but don't provide Access-Control headers (test that the font doesn't get applied).

If you just add a reftest or crashtest we run all of those while checking for leaks.

Comment 10

[John Daggett (:jtd)]

Attachment: patch, v.0.2a, add failing crashtest test

Add an entry in the crashtests for layout/style to intentionally cause a cross-site access load that will fail.  Also, add outer notification callbacks to objects dropped on failure.

Comment 11

[John Daggett (:jtd)]

Pushed to mozcentral
http://hg.mozilla.org/mozilla-central/rev/d27323792a59

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Did this actually cause some new leaks on
OS X 10.5.2 mozilla-central unit test
and
WINNT 5.2 mozilla-central unit test?

Or perhaps the leaks are something random.