760342 - Add debug check for HashTable::Enum incorrect usage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

If we removeFront or rekeyFront on an enum, further access to enum.front() is bad.  With removeFront, this will just fail immediately, but with rekeyFront, it is possible that this will not trigger any obviously bad behavior in many cases.  This DebugOnly to explicitly tests for incorrect usage and aborts immediately.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 629025 [details] [diff] [review]
v0

Review of attachment 629025 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/HashTable.h
@@ +207,5 @@
>           */
>          void removeFront() {
>              table.remove(*this->cur);
>              removed = true;
> +            this->validEntry = false;

no this->

@@ +224,5 @@
>              HashPolicy::setKey(e.t, const_cast<Key &>(k));
>              table.remove(*this->cur);
>              table.add(l, e);
>              added = true;
> +            this->validEntry = false;

no this->

Comment 2

[Terrence Cole [:terrence]]

(In reply to Luke Wagner [:luke] from comment #1)
It doesn't compile without those this->.

./dist/include/js/HashTable.h: In member function ‘void js::detail::HashTable<T, HashPolicy, AllocPolicy>::Enum::rekeyFront(js::detail::HashTable<T, HashPolicy, AllocPolicy>::Lookup&, js::detail::HashTable<T, HashPolicy, AllocPolicy>::Key&)’:
./dist/include/js/HashTable.h:228:13: error: ‘validEntry’ was not declared in this scope

I've bumped into this before.  What's going on here?

Comment 3

[Luke Wagner [:luke]]

Ohhh, I didn't notice that the accesses via this-> were members of the base class.  This is the same reoccurring problem where two-phase template type-checking wants you to make syntactically evident which names are base member names.

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57f92b0c3097

Comment 5

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back this out on inbound because of broken builds on Windows:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b8fe3834b9
https://tbpl.mozilla.org/php/getParsedLog.php?id=12291968&tree=Mozilla-Inbound

Comment 6

[Phil Ringnalda (:philor)]

Pretty sure that was actually from bug 751995 - if I'm right once the dust settles, I'll reland this.

Comment 7

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/70288798e65f - just be careful about keeping such bad company in the future ;)

Comment 8

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/57f92b0c3097

Comment 9

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/d7b8fe3834b9

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/70288798e65f