Closed Bug 760342 Opened 7 years ago Closed 7 years ago

Add debug check for HashTable::Enum incorrect usage

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(1 file)

Attached patch v0Splinter Review
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.
Attachment #629025 - Flags: review?(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->
Attachment #629025 - Flags: review?(luke) → review+
(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?
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.
Pretty sure that was actually from bug 751995 - if I'm right once the dust settles, I'll reland this.
Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/70288798e65f - just be careful about keeping such bad company in the future ;)
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/57f92b0c3097
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Backed out: https://hg.mozilla.org/mozilla-central/rev/d7b8fe3834b9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/70288798e65f
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.