726334 - nsFormFillController::mFocusedInput can keep nodes alive longer than needed

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

If I read the code correctly nsFormFillController::mFocusedInput can
actually in some cases keep node alive longer than needed.

Comment 1

[(dormant account)]

(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)]

(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][[e-mail]]]

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)]

(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)]

I keep accidentally using a non-existent snappy:p4 priority

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

Bah, looks like the patch crashes nicely :(

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better patch

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=842460184f27

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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: [e-mail]]]

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][[e-mail]]]

(In reply to Gavin Sharp (use [e-mail] 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][[e-mail]]]

(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: [e-mail]]]

(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][[e-mail]]]

Attachment: patch

Contains is inclusive
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-node-contains

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 599413 [details] [diff] [review]
patch

thanks for fixing this

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/2d065305b7d2

Thanks for quick review!