Closed
Bug 831360
Opened 12 years ago
Closed 7 years ago
Make nsPresContext a skippable class
Categories
(Core :: DOM: CSS Object Model, defect)
Core
DOM: CSS Object Model
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(2 files, 3 obsolete files)
41.84 KB,
application/pdf
|
Details | |
2.84 KB,
patch
|
Details | Diff | Splinter Review |
I have a CC log here with 187 nsDOMMediaQueryList, all pointing to an nsPresContext. I'm not quite sure what the ownership for these is, but perhaps we can filter them out of the CC graph when they are obviously live.
Assignee | ||
Comment 1•12 years ago
|
||
Do we have a way of telling that a pres context is obviously live? If so, it should be straightforward if we do; the nsDOMMediaQueryList objects should be referenced either from JS or (when they have listeners) from the pres context.
What mechanism do you suggest for "filtering them out"? Is the idea that you don't traverse *to* them or that you do something in their own cycle collection methods?
Component: Layout → DOM: CSS Object Model
Comment 3•12 years ago
|
||
if PresContext::mDocument is in CC-generation and mDocument->GetShell()->GetPresContext() == this,
Prescontext should be certainly alive, I think.
Assignee | ||
Comment 4•12 years ago
|
||
> Is the idea that you don't traverse *to* them or that you do something in their own cycle collection methods?
You can define CAN_SKIP methods that returns true if the object is definitely alive, and that will be called at various points by the CC to see if it can be skipped.
It seems like maybe we can only tell if it has listeners? I'd be wary of the case when it is owned by JS, as that can often create arbitrary cycles.
Assignee | ||
Comment 5•12 years ago
|
||
I'm not sure which case this actually was, though. It may not be worth investigating further unless I see this again.
Assignee | ||
Comment 6•12 years ago
|
||
I threw this patch together, but unfortunately none of the dozens of media lists on TechCrunch have listeners, so it doesn't do anything. I'm not sure how to get ahold of the JS that owns these lists. If I could do that, then I could see if they are marked black.
Assignee | ||
Comment 7•12 years ago
|
||
Changing CAN_SKIP to this eliminates constant CCs on TechCrunch:
return !tmp->mPresContext || tmp->mPresContext->OwnedByInCCGenerationDocument();
My reasoning is that if we have listeners, then we're owned by our mPresContext. If we don't have listeners, then the only thing we're going to traverse is the pres context, so if the pres context isn't garbage, then we won't end up doing anything.
On further reflection, I think if we just make nsPresContext skippable, using the OwnedByInCCGenerationDocument(), then the ChildFinder logic in the CC will handle this case without requiring any changes to nsDOMMediaQueryList, which would be better. I'll try that.
Assignee | ||
Comment 8•12 years ago
|
||
This also fixes the TechCrunch problem.
Attachment #706683 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
Summary: Don't traverse live nsDOMMediaQueryList in CC → Make nsPresContext a skippable class
Assignee | ||
Comment 10•12 years ago
|
||
I don't know if dbaron should review this or not. I guess it depends on how confident you are in your knowledge of nsPresContext ownership, Olli. ;)
In this version of the patch, I don't add a method to nsPresContext, because we aren't actually accessing any private data. Using Document() instead of directly accessing the member is probably a better idea anyways.
Assignee: nobody → continuation
Attachment #706709 -
Attachment is obsolete: true
Attachment #710105 -
Flags: review?(bugs)
Assignee | ||
Comment 11•12 years ago
|
||
I removed the trailing whitespace.
Attachment #710105 -
Attachment is obsolete: true
Attachment #710105 -
Flags: review?(bugs)
Attachment #710107 -
Flags: review?(bugs)
Comment 12•12 years ago
|
||
Comment on attachment 710107 [details] [diff] [review]
make nsPresContext a skippable class
Too hard... I can't prove that this is right, but I can't say it isn't right either. I'll try to figure this out.
Attachment #710107 -
Flags: review?(bugs)
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•