707862 - Reset childCount on SHEntry when all children have been removed

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Follow-up from bug 705597 comment #43:

Removing children from a SHEntry creates holes. After all children have been removed the .childCount property should be reset to zero. Otherwise it indicates there are SHEntry children but will return null for every GetChildAt() call.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

I realized that we could go even further and always remove empty children when they're at the end of the mChildren array - that's what this patch does.

Regarding the test, I didn't know where a test like this should go and if there are any SHistory mochitests? I for now chose the sessionstore component because the test is almost identical to the one from bug 705597 (only slightly adapted).

Comment 2

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

There are quite a few tests in docshell/test

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Oops, I looked for them in docshell/shistory/... Will move the test.

Comment 4

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

I'm trying to remember if we can cleanup mChildren, especially if there are some
non-dynamically added children and some dynamically added.

Comment 5

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

How often does the situation actually happen?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

I have no idea how often that happens in the wild. It seems to happen but maybe not really often or only with some sites. In fact, since bug 705597 landed it doesn't affect session store anymore so if you're not sure about the consequences of resetting childCount I'm fine with not doing it. Just wanted to try fixing 'the real issue' instead of just the symptom :)

Comment 7

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

Comment on attachment 580643 [details] [diff] [review]
patch v1

Please land this very early in FF13 cycle. Session history changes
are very regression risky. If this causes regressions, please 
back out asap.

Comment 8

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

Comment on attachment 580643 [details] [diff] [review]
patch v1

And use --i, not i--

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #8)
> And use --i, not i--

Fixed.

https://hg.mozilla.org/integration/fx-team/rev/d8b2e5047c9b

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d8b2e5047c9b