The default bug view has changed. See this FAQ.

Reset childCount on SHEntry when all children have been removed

RESOLVED FIXED in mozilla13

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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).
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Attachment #580643 - Flags: review?(bugs)
There are quite a few tests in docshell/test
(Assignee)

Comment 3

5 years ago
Oops, I looked for them in docshell/shistory/... Will move the test.
I'm trying to remember if we can cleanup mChildren, especially if there are some
non-dynamically added children and some dynamically added.
How often does the situation actually happen?
(Assignee)

Comment 6

5 years ago
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 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.
Attachment #580643 - Flags: review?(bugs) → review+
Comment on attachment 580643 [details] [diff] [review]
patch v1

And use --i, not i--
(Assignee)

Comment 9

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

Fixed.

https://hg.mozilla.org/integration/fx-team/rev/d8b2e5047c9b
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla13
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d8b2e5047c9b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.