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

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: jtd)

Tracking

({memory-leak, testcase})

Trunk
x86
Mac OS X
memory-leak, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
Created attachment 358073 [details]
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:?
(Assignee)

Comment 2

9 years ago
Created attachment 359477 [details] [diff] [review]
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.
Assignee: nobody → jdaggett
Attachment #359477 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Priority: -- → P3
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 7

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

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+
(Assignee)

Comment 10

9 years ago
Created attachment 359889 [details] [diff] [review]
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.
(Assignee)

Comment 11

9 years ago
Pushed to mozcentral
http://hg.mozilla.org/mozilla-central/rev/d27323792a59
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.