838304 - null out mResult when removing container nodes

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

OnRemoving of container nodes I think we should null out mResult, like we do with mParent.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/890368240f48

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e059622a0b3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e059622a0b3