Last Comment Bug 707862 - Reset childCount on SHEntry when all children have been removed
: Reset childCount on SHEntry when all children have been removed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 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 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-12-10 09:29:19 PST
There are quite a few tests in docshell/test
Comment 3 Tim Taubert [:ttaubert] 2011-12-10 09:31:25 PST
Oops, I looked for them in docshell/shistory/... Will move the test.
Comment 4 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 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 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-09 10:26:39 PST
How often does the situation actually happen?
Comment 6 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 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 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 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-31 05:00:55 PST
Comment on attachment 580643 [details] [diff] [review]
patch v1

And use --i, not i--
Comment 9 Tim Taubert [:ttaubert] 2012-02-01 02:53:21 PST
(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] 2012-02-02 01:08:57 PST
https://hg.mozilla.org/mozilla-central/rev/d8b2e5047c9b

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