Closed
Bug 760342
Opened 12 years ago
Closed 12 years ago
Add debug check for HashTable::Enum incorrect usage
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: terrence, Assigned: terrence)
Details
Attachments
(1 file)
3.88 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter 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 1•12 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•12 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•12 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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f92b0c3097
Comment 5•12 years ago
|
||
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•12 years ago
|
||
Pretty sure that was actually from bug 751995 - if I'm right once the dust settles, I'll reland this.
Comment 7•12 years ago
|
||
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
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/57f92b0c3097
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 9•12 years ago
|
||
Backed out: https://hg.mozilla.org/mozilla-central/rev/d7b8fe3834b9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/70288798e65f
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•