Last Comment Bug 772292 - Convert JSObject2WrappedJSMap to a new-style HashTable
: Convert JSObject2WrappedJSMap to a new-style HashTable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
: xpconnect
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-09 17:06 PDT by Terrence Cole [:terrence]
Modified: 2012-07-20 06:46 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (13.40 KB, patch)
2012-07-09 17:06 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v1: With fixes. (13.50 KB, patch)
2012-07-10 10:03 PDT, Terrence Cole [:terrence]
mrbkap: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-07-09 17:06:11 PDT
Created attachment 640441 [details] [diff] [review]
v0

Try run up at:
https://tbpl.mozilla.org/?tree=Try&rev=ba57767df1f9
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-07-10 01:32:52 PDT
Comment on attachment 640441 [details] [diff] [review]
v0

Review of attachment 640441 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCMaps.cpp
@@ +93,5 @@
> +                if (JS_IsAboutToBeFinalized(wrapper->GetJSObjectPreserveColor()))
> +                    dying->AppendElement(wrapper);
> +            }
> +        }
> +        wrapper = wrapper->GetNextWrapper();

This line should be inside the while loop.

::: js/xpconnect/src/XPCMaps.h
@@ +604,5 @@
>      typedef js::HashMap<JSObject *, JSObject *, js::PointerHasher<JSObject *, 3>,
>                          js::SystemAllocPolicy> Map;
>  
>  public:
> +    static JSObject2JSObjectMap* newMap(int size) {

Unrelated?

@@ +664,5 @@
>      }
>  
>  private:
>      JSObject2JSObjectMap() MOZ_DELETE;
> +    JSObject2JSObjectMap(int size) {}

Should remove this constructor, then.
Comment 2 Terrence Cole [:terrence] 2012-07-10 10:03:20 PDT
Created attachment 640653 [details] [diff] [review]
v1: With fixes.

(In reply to :Ms2ger from comment #1)
> ::: js/xpconnect/src/XPCMaps.cpp
> @@ +93,5 @@
> > +                if (JS_IsAboutToBeFinalized(wrapper->GetJSObjectPreserveColor()))
> > +                    dying->AppendElement(wrapper);
> > +            }
> > +        }
> > +        wrapper = wrapper->GetNextWrapper();
> 
> This line should be inside the while loop.

Good catch.  I made the mistake of trying to understand what this loop was doing instead of just copying it.
 
> ::: js/xpconnect/src/XPCMaps.h
> @@ +604,5 @@
> >      typedef js::HashMap<JSObject *, JSObject *, js::PointerHasher<JSObject *, 3>,
> >                          js::SystemAllocPolicy> Map;
> >  
> >  public:
> > +    static JSObject2JSObjectMap* newMap(int size) {
> 
> Unrelated?

A cleanup of the previous jsdhash I squashed.  I figured while I was here I would clean this up a bit more.

> @@ +664,5 @@
> >      }
> >  
> >  private:
> >      JSObject2JSObjectMap() MOZ_DELETE;
> > +    JSObject2JSObjectMap(int size) {}
> 
> Should remove this constructor, then.

Was the whole point of the above change.  Did it for the JSObject2WrappedJS but forgot it here for some reason.
Comment 3 Terrence Cole [:terrence] 2012-07-10 10:05:36 PDT
Previous try run appears to have happened in the middle of complete infrastructure havoc:
https://tbpl.mozilla.org/?tree=Try&rev=08c73baedbee
Comment 4 Blake Kaplan (:mrbkap) 2012-07-17 18:50:08 PDT
Comment on attachment 640653 [details] [diff] [review]
v1: With fixes.

Review of attachment 640653 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCMaps.h
@@ -74,5 @@
>  
> -    ~JSObject2WrappedJSMap();
> -private:
> -    JSObject2WrappedJSMap();    // no implementation
> -    JSObject2WrappedJSMap(int size);

Should the constructor stay private here to make sure consumers use newMap?

@@ -668,5 @@
>          }
>      }
>  
>  private:
> -    JSObject2JSObjectMap() MOZ_DELETE;

Ditto here.
Comment 5 Terrence Cole [:terrence] 2012-07-19 09:39:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff15e729ec86
Comment 6 Ed Morley [:emorley] 2012-07-20 06:46:59 PDT
https://hg.mozilla.org/mozilla-central/rev/ff15e729ec86

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