Closed
Bug 474704
Opened 16 years ago
Closed 16 years ago
[@font-face] Leak nsCrossSiteListenerProxy and nsFontFaceLoader (loading 472237-1.html over file:)
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: jtd)
Details
(Keywords: memory-leak, testcase)
Attachments
(4 files)
3.18 KB,
text/plain
|
Details | |
1.70 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.38 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
4.02 KB,
patch
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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•16 years ago
|
Priority: -- → P3
Assignee | ||
Updated•16 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:)
![]() |
||
Comment 3•16 years ago
|
||
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•16 years ago
|
||
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 6•16 years ago
|
||
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•16 years ago
|
||
Jonas, does this look right to you?
Attachment #359726 -
Flags: review?(jonas)
![]() |
||
Comment 8•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
Pushed to mozcentral
http://hg.mozilla.org/mozilla-central/rev/d27323792a59
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 12•16 years ago
|
||
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.
Description
•