Last Comment Bug 707862 - Reset childCount on SHEntry when all children have been removed
: Reset childCount on SHEntry when all children have been removed
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla13
Assigned To: Tim Taubert [:ttaubert]
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2011-12-05 18:57 PST by Tim Taubert [:ttaubert]
Modified: 2012-02-02 01:08 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (3.47 KB, patch)
2011-12-10 06:19 PST, Tim Taubert [:ttaubert]
bugs: review+
Details | Diff | Splinter Review

Description User image Tim Taubert [:ttaubert] 2011-12-05 18:57:44 PST
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 User image Tim Taubert [:ttaubert] 2011-12-10 06:19:03 PST
Created attachment 580643 [details] [diff] [review]
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 User image Olli Pettay [:smaug] 2011-12-10 09:29:19 PST
There are quite a few tests in docshell/test
Comment 3 User image Tim Taubert [:ttaubert] 2011-12-10 09:31:25 PST
Oops, I looked for them in docshell/shistory/... Will move the test.
Comment 4 User image Olli Pettay [:smaug] 2011-12-10 09:37:51 PST
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 User image Olli Pettay [:smaug] 2012-01-09 10:26:39 PST
How often does the situation actually happen?
Comment 6 User image Tim Taubert [:ttaubert] 2012-01-13 04:00:06 PST
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 User image Olli Pettay [:smaug] 2012-01-31 05:00:06 PST
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 User image Olli Pettay [:smaug] 2012-01-31 05:00:55 PST
Comment on attachment 580643 [details] [diff] [review]
patch v1

And use --i, not i--
Comment 9 User image Tim Taubert [:ttaubert] 2012-02-01 02:53:21 PST
(In reply to Olli Pettay [:smaug] from comment #8)
> And use --i, not i--

Comment 10 User image Tim Taubert [:ttaubert] 2012-02-02 01:08:57 PST

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