Closed Bug 838304 Opened 8 years ago Closed 8 years ago

null out mResult when removing container nodes

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21

People

(Reporter: mak, Assigned: mak)

Details

Attachments

(1 file, 1 obsolete file)

patch v1.1
6.33 KB, patch
mano
: review+
Details | Diff | Splinter Review 
OnRemoving of container nodes I think we should null out mResult, like we do with mParent.
Attached patch patch v1.0 (obsolete) — — Splinter Review
Attachment #710338 - Flags: review?(mano)
backed out, looks like some of our xpcshell tests rely on this, scary!

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f50b3e8bb1

TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/bookmarks/test_423515_forceCopyShortcuts.js
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_history_sidebar.js
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/unit/test_tagging.js

Looking into them.
Target Milestone: mozilla21 → ---
ah looks like it's just an annoying wrong assertion
 ASSERTION: Containers must have valid results: 'container->mResult'

the truth is that either mParent and mResult are valid, or are invalid, if they are invalid the node is a zombie, nothing wrong with that.
Attached patch patch v1.1 — — Splinter Review 
a bit more changes than expected so reasking for review, tests are happy locally.
Looks like in the past we lived in an epoch where result was an invariant for containers, but most of the code null checks it, and I think it's healthier to do so than to keep a result around.
Attachment #710338 - Attachment is obsolete: true
Attachment #710413 - Flags: review?(mano)
https://hg.mozilla.org/mozilla-central/rev/1e059622a0b3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.