Last Comment Bug 760342 - Add debug check for HashTable::Enum incorrect usage
: Add debug check for HashTable::Enum incorrect usage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-31 19:00 PDT by Terrence Cole [:terrence]
Modified: 2012-06-02 12:25 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (3.88 KB, patch)
2012-05-31 19:00 PDT, Terrence Cole [:terrence]
luke: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-05-31 19:00:52 PDT
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.
Comment 1 Luke Wagner [:luke] 2012-05-31 19:13:27 PDT
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] 2012-06-01 10:03:12 PDT
(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] 2012-06-01 13:45:45 PDT
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] 2012-06-01 15:44:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f92b0c3097
Comment 5 Matt Brubeck (:mbrubeck) 2012-06-01 17:45:34 PDT
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) 2012-06-01 18:33:26 PDT
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) 2012-06-01 19:46:11 PDT
Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/70288798e65f - just be careful about keeping such bad company in the future ;)
Comment 9 :Ehsan Akhgari 2012-06-02 12:23:17 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/d7b8fe3834b9

Note You need to log in before you can comment on or make changes to this bug.