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]
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 | Review

Description 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 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 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
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:
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 - just be careful about keeping such bad company in the future ;)
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 12:16:41 PDT
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 12:23:17 PDT
Backed out:
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 12:25:19 PDT

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