Closed Bug 728577 Opened 10 years ago Closed 10 years ago

nsGlobalWindow::mFocusedNode seems to keep documents alive longer than needed

Categories

(Core :: DOM: Core & HTML, defect)

12 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox12 + fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa?])

Attachments

(1 file, 1 obsolete file)

I've seen cc logs where globalwindow doesn't have any other child than mFocusedNode.
That sounds like mFocusedNode is re-set after unlink.
Attached patch WIP (obsolete) — Splinter Review
https://hg.mozilla.org/try/rev/da3e470f9b39

I'm using the patch for some time to see if it helps.
Comment on attachment 598549 [details] [diff] [review]
WIP

Neil, what do you think about this.
Trying to be more careful when setting a strong member variable.
I should probably add still one check
if (aNode && (!aNode->IsInDoc || aNode->GetCurrentDoc() != mDoc)) {

I wish I could reproduce the problematic case.
Attachment #598549 - Flags: review?(enndeakin)
Better patch coming.
We should also release mFocusedNode whenever window doesn't point to that document anymore.
Attached patch patchSplinter Review
Assignee: nobody → bugs
Attachment #598549 - Attachment is obsolete: true
Attachment #599582 - Flags: review?(enndeakin)
Attachment #598549 - Flags: review?(enndeakin)
Duplicate of this bug: 727249
So all these places where mFocusedNode is set to null are all points where the window or document is no longer valid for display?

If calling SetFocusedNode(somenode) is being called on an 'cleaned up' window, do we still want that window to be focused?
(In reply to Neil Deakin from comment #7)
> So all these places where mFocusedNode is set to null are all points where
> the window or document is no longer valid for display?
When window doesn't own the document anymore.
The window may start owning some other document.


> If calling SetFocusedNode(somenode) is being called on an 'cleaned up'
> window, do we still want that window to be focused?
No. Cleaned up window is really a window to be deleted soon.
(In reply to Olli Pettay [:smaug] from comment #8)
> > If calling SetFocusedNode(somenode) is being called on an 'cleaned up'
> > window, do we still want that window to be focused?
> No. Cleaned up window is really a window to be deleted soon.

So why is SetFocusedNode (or TakeFocus) being called on a window that is being deleted? nsFocusManager::WindowHidden should have been called on that window beforehand.
As you know, SetFocusedNode is being called on wrong window occasionally (there are bugs open).
Those bugs track the reason for that. Once they are fixed, we can make the NS_ASSERTION/NS_WARNING
MOZ_ASSERT.
For example https://bugzilla.mozilla.org/show_bug.cgi?id=561940

This bug is about making sure we don't keep documents alive via mFocusedNode, since that
make cycle collector times significantly worse.
Attachment #599582 - Flags: review?(enndeakin) → review+
Thanks!
Tryserver looked good.

https://hg.mozilla.org/mozilla-central/rev/bd6567f435bf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 728549
Target Milestone: --- → mozilla13
The patch applies to FF12 too.
Comment on attachment 599582 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Higher cycle collection times
Testing completed (on m-c, etc.): in m-c since 2012-02-22
Risk to taking this patch (and alternatives if risky): Should be low risky
String changes made by this patch: N/A
Attachment #599582 - Flags: approval-mozilla-aurora?
Comment on attachment 599582 [details] [diff] [review]
patch

[Triage Comment]
Approving for Aurora 12 in defense of recent internally reported GC/CC pauses. Deemed low risk.
Attachment #599582 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is there a reproducible testcase QA can use to verify this fix?
Whiteboard: [qa?]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.