Last Comment Bug 726334 - nsFormFillController::mFocusedInput can keep nodes alive longer than needed
: nsFormFillController::mFocusedInput can keep nodes alive longer than needed
Status: RESOLVED FIXED
[snappy:p3][MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: Form Manager (show other bugs)
: 12 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla13
Assigned To: Olli Pettay [:smaug] (TPAC)
:
Mentors:
Depends on: 730470
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-11 11:24 PST by Olli Pettay [:smaug] (TPAC)
Modified: 2012-02-24 23:33 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.29 KB, patch)
2012-02-20 16:38 PST, Olli Pettay [:smaug] (TPAC)
no flags Details | Diff | Splinter Review
better patch (12.89 KB, patch)
2012-02-20 17:10 PST, Olli Pettay [:smaug] (TPAC)
no flags Details | Diff | Splinter Review
patch (13.27 KB, patch)
2012-02-21 11:50 PST, Olli Pettay [:smaug] (TPAC)
gavin.sharp: review-
Details | Diff | Splinter Review
patch (15.29 KB, patch)
2012-02-21 16:55 PST, Olli Pettay [:smaug] (TPAC)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (TPAC) 2012-02-11 11:24:39 PST
If I read the code correctly nsFormFillController::mFocusedInput can
actually in some cases keep node alive longer than needed.
Comment 1 (dormant account) 2012-02-14 14:59:32 PST
(In reply to Olli Pettay [:smaug] from comment #0)
> If I read the code correctly nsFormFillController::mFocusedInput can
> actually in some cases keep node alive longer than needed.

how often does this happen?
Comment 2 Mounir Lamouri (:mounir) 2012-02-14 15:05:27 PST
(In reply to Olli Pettay [:smaug] from comment #0)
> If I read the code correctly nsFormFillController::mFocusedInput can
> actually in some cases keep node alive longer than needed.

Could you be more specific?
Comment 3 Olli Pettay [:smaug] (TPAC) 2012-02-14 15:32:48 PST
Something like hiding focused input element and moving it to another document might
cause this to happen.
I don't know how often this happens, since this is still speculation.
Something is keeping input elements alive occasionally, though.
Comment 4 (dormant account) 2012-02-14 15:53:34 PST
(In reply to Olli Pettay [:smaug] from comment #3)
> Something like hiding focused input element and moving it to another
> document might
> cause this to happen.
> I don't know how often this happens, since this is still speculation.
> Something is keeping input elements alive occasionally, though.

We need a needs-telemetry whiteboard entry. In meantime I'm P4ing this until telemetry(or something) can show us extent of this problem.
Comment 5 (dormant account) 2012-02-14 16:11:05 PST
I keep accidentally using a non-existent snappy:p4 priority
Comment 6 Olli Pettay [:smaug] (TPAC) 2012-02-20 16:38:30 PST
Created attachment 599005 [details] [diff] [review]
patch

I hadn't realized formfillcontroller is a service, which makes the
situation even worse.

Chrome code really shouldn't keep strong references to content nodes.
It is just way too easy to leak some of them accidentally.
I'll ask review if the tryserver results look good.

https://tbpl.mozilla.org/?tree=Try&rev=915c6d323e19
Comment 7 Olli Pettay [:smaug] (TPAC) 2012-02-20 16:46:58 PST
Bah, looks like the patch crashes nicely :(
Comment 8 Olli Pettay [:smaug] (TPAC) 2012-02-20 17:10:04 PST
Created attachment 599013 [details] [diff] [review]
better patch
Comment 9 Olli Pettay [:smaug] (TPAC) 2012-02-20 17:15:38 PST
https://tbpl.mozilla.org/?tree=Try&rev=842460184f27
Comment 10 Olli Pettay [:smaug] (TPAC) 2012-02-21 11:50:16 PST
Created attachment 599284 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=ee6e64a48620
This should actually pass the input.list tests.
Need to be careful with mListNode handling so that we always have pointer to it
when needed.

The patch makes FFC to never own any dom nodes. All the nodes it has pointer to
are controlled via mutationobservers.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-21 14:08:52 PST
Comment on attachment 599284 [details] [diff] [review]
patch

nit: I noticed a couple of "if("s (missing space)

If you're going to start adding the formfillcontroller as an nsIMutationObserver for more than just list elements (i.e. also for the inputs themselves), you'll need to add a checks to avoid calling RevalidateDataList() for changes to the <input>. r- because of that.

>diff --git a/toolkit/components/satchel/nsFormFillController.cpp b/toolkit/components/satchel/nsFormFillController.cpp

> nsFormFillController::~nsFormFillController()

>+  if (mListNode) {

>+  if (mFocusedInputNode) {

These changes shouldn't be necessary since RemoveWindowListeners (called later in this function) calls StopControllingInput(). In theory that also takes care of clearing mPwmgrInputs, though only if the relevant windows still have documents, which might not be as reliable, I guess?

>diff --git a/toolkit/components/satchel/nsFormFillController.h b/toolkit/components/satchel/nsFormFillController.h

>+  nsDataHashtable<nsPtrHashKey<const nsINode>, PRInt32> mPwmgrInputs;

Might as well make this nsDataHashtable<nsPtrHashKey<const nsINode>, bool>, no?
Comment 12 Olli Pettay [:smaug] (TPAC) 2012-02-21 14:12:23 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11)
> Comment on attachment 599284 [details] [diff] [review]
> patch
> 
> nit: I noticed a couple of "if("s (missing space)

Hmm, where did those come from. strange.

> 
> If you're going to start adding the formfillcontroller as an
> nsIMutationObserver for more than just list elements (i.e. also for the
> inputs themselves), you'll need to add a checks to avoid calling
> RevalidateDataList() for changes to the <input>. r- because of that.
Why?

> These changes shouldn't be necessary since RemoveWindowListeners (called
> later in this function) calls StopControllingInput(). In theory that also
> takes care of clearing mPwmgrInputs, though only if the relevant windows
> still have documents, which might not be as reliable, I guess?
Yeah.

> Might as well make this nsDataHashtable<nsPtrHashKey<const nsINode>, bool>,
> no?
Sure, I just didn't change the existing value type.
Comment 13 Olli Pettay [:smaug] (TPAC) 2012-02-21 14:17:10 PST
(In reply to Olli Pettay [:smaug] from comment #12)
 
> > These changes shouldn't be necessary since RemoveWindowListeners (called
> > later in this function) calls StopControllingInput(). In theory that also
> > takes care of clearing mPwmgrInputs, though only if the relevant windows
> > still have documents, which might not be as reliable, I guess?
Actually, that is not enough. I don't see what guarantees that mDocShells has any items.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-21 15:22:58 PST
(In reply to Olli Pettay [:smaug] from comment #13)
> Actually, that is not enough. I don't see what guarantees that mDocShells
> has any items.

You're right - it's theoretically possible for mPwmgrInputs to be populated without AttachToBrowser having been called. I don't think that happens in practice because form fill controller is always hooked up to every docshell in Firefox (even if form filling is disabled). I guess that's a good thing since we'd otherwise leak all these inputs indefinitely...
Comment 15 Olli Pettay [:smaug] (TPAC) 2012-02-21 16:55:11 PST
Created attachment 599413 [details] [diff] [review]
patch

Contains is inclusive
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-node-contains
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-21 17:29:21 PST
Comment on attachment 599413 [details] [diff] [review]
patch

thanks for fixing this
Comment 17 Olli Pettay [:smaug] (TPAC) 2012-02-22 05:19:00 PST
https://hg.mozilla.org/mozilla-central/rev/2d065305b7d2

Thanks for quick review!

Note You need to log in before you can comment on or make changes to this bug.