Add debug check for HashTable::Enum incorrect usage

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 629025 [details] [diff] [review]
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.
Attachment #629025 - Flags: review?(luke)

Comment 1

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

Comment 2

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

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

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f92b0c3097
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
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
Last Resolved: 5 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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.