Last Comment Bug 760342 - Add debug check for HashTable::Enum incorrect usage
: Add debug check for HashTable::Enum incorrect usage
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla15
Assigned To: Terrence Cole [:terrence]
: Jason Orendorff [:jorendorff]
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image Terrence Cole [:terrence] 2012-05-31 19:00:52 PDT
Created attachment 629025 [details] [diff] [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.
Comment 1 User image Luke Wagner [:luke] 2012-05-31 19:13:27 PDT
Comment on attachment 629025 [details] [diff] [review]

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 User image 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 User image 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 User image Terrence Cole [:terrence] 2012-06-01 15:44:34 PDT
Comment 5 User image 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:
Comment 6 User image 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 User image Phil Ringnalda (:philor) 2012-06-01 19:46:11 PDT
Relanded in - just be careful about keeping such bad company in the future ;)
Comment 9 User image :Ehsan Akhgari 2012-06-02 12:23:17 PDT
Backed out:

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