Closed Bug 474704 Opened 14 years ago Closed 14 years ago

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


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






(Reporter: jruderman, Assigned: jtd)


(Keywords: memory-leak, testcase)


(4 files)

Attached file 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.
Do any of the content/base/test tests leak when run from file:?
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.
Assignee: nobody → jdaggett
Attachment #359477 - Flags: review?(dbaron)
Priority: -- → P3
Summary: Leak nsCrossSiteListenerProxy and nsFontFaceLoader (loading 472237-1.html over file:) → [@font-face] Leak nsCrossSiteListenerProxy and nsFontFaceLoader (loading 472237-1.html over file:)
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?
In particular, if I'm correct there's no cycle to be broken if allocating the proxy fails, right?
Attachment #359477 - Flags: review?(dbaron) → review?(bzbarsky)
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 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.
Attachment #359477 - Flags: review?(bzbarsky) → review+
Jonas, does this look right to you?
Attachment #359726 - Flags: review?(jonas)
You probably want to drop the outer notification callbacks too, right?
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.
Attachment #359726 - Flags: superreview+
Attachment #359726 - Flags: review?(jonas)
Attachment #359726 - Flags: review+
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.
Pushed to mozcentral
Closed: 14 years ago
Resolution: --- → FIXED
Did this actually cause some new leaks on
OS X 10.5.2 mozilla-central unit test
WINNT 5.2 mozilla-central unit test?

Or perhaps the leaks are something random.
You need to log in before you can comment on or make changes to this bug.