The default bug view has changed. See this FAQ.

Convert JSObject2WrappedJSMap to a new-style HashTable

RESOLVED FIXED in mozilla17

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 640441 [details] [diff] [review]
v0

Try run up at:
https://tbpl.mozilla.org/?tree=Try&rev=ba57767df1f9
Attachment #640441 - Flags: review?(mrbkap)
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.
(Assignee)

Comment 2

5 years ago
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.
Attachment #640441 - Attachment is obsolete: true
Attachment #640441 - Flags: review?(mrbkap)
Attachment #640653 - Flags: review?(mrbkap)
(Assignee)

Comment 3

5 years ago
Previous try run appears to have happened in the middle of complete infrastructure havoc:
https://tbpl.mozilla.org/?tree=Try&rev=08c73baedbee
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.
Attachment #640653 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff15e729ec86
https://hg.mozilla.org/mozilla-central/rev/ff15e729ec86
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.