Last Comment Bug 776159 - Thread more handles through jsarray
: Thread more handles through jsarray
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
: general
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-20 16:25 PDT by Terrence Cole [:terrence]
Modified: 2012-07-26 05:08 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (16.54 KB, patch)
2012-07-20 16:25 PDT, Terrence Cole [:terrence]
sphink: review+
Details | Diff | Splinter Review
v1 (14.39 KB, patch)
2012-07-23 11:56 PDT, Terrence Cole [:terrence]
sphink: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-07-20 16:25:23 PDT
Created attachment 644519 [details] [diff] [review]
v0

Steve, do these changes make sense, or am I being too aggressive?
Comment 1 Steve Fink [:sfink] [:s:] 2012-07-23 09:52:38 PDT
Comment on attachment 644519 [details] [diff] [review]
v0

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

::: js/src/jsarray.cpp
@@ +3742,5 @@
>          }
>      }
>  
>      Rooted<GlobalObject*> parent(cx, parent_);
> +    RootedObject proto(cx, protoArg);

Ooh, that's a good example of a tricky case. You're double-rooting proto, but I think it's probably best this way. The inner proto that you pass to FindProto definitely has to be rooted, but the parameter doesn't need to be -- looking at the callers, you're now explicitly rooting and so you could avoid one root by passing in a bare JSObject* and rooting it at the top. But I like it the way you did it better, just for the simplicity; you don't have to think it through this way.

It's a good example for a rooting document.
Comment 2 Bill McCloskey (:billm) 2012-07-23 10:46:34 PDT
Comment on attachment 644519 [details] [diff] [review]
v0

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

::: js/src/jsarray.cpp
@@ +3795,5 @@
>  #ifdef JS_METHODJIT
>  JSObject * JS_FASTCALL
>  mjit::stubs::NewDenseUnallocatedArray(VMFrame &f, uint32_t length)
>  {
> +    JSObject *obj = NewArray<false>(f.cx, length, HandleObject::fromMarkedLocation((JSObject **)&f.scratch));

Is this safe? f.scratch is not a marked location, as far as I can tell.
Comment 3 Terrence Cole [:terrence] 2012-07-23 11:56:28 PDT
Created attachment 645010 [details] [diff] [review]
v1

Steve, I've updated this to use RawObject, and I think it's better overall now. I do want a second opinion, however, so I'm re-requesting.

(In reply to Bill McCloskey (:billm) from comment #2)
> HandleObject::fromMarkedLocation((JSObject **)&f.scratch));
> 
> Is this safe? f.scratch is not a marked location, as far as I can tell.

I assumed wrong.  Do we need these to be marked through for moving GC?
Comment 4 Steve Fink [:sfink] [:s:] 2012-07-24 15:21:08 PDT
Comment on attachment 645010 [details] [diff] [review]
v1

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

Sorry about missing the f.scratch thing earlier; I made the wrong assumption too. I see now that it is a void* slot. Unfortunately, it seems to be used to store things like argc at times, so we can't just change it to a gc::Cell* and trace it.

::: js/src/jsarray.cpp
@@ +3683,5 @@
>      }
>  
>      Rooted<GlobalObject*> parent(cx, parent_);
> +    RootedObject proto(cx, protoArg);
> +    PoisonPtr(reinterpret_cast<uintptr_t *>(protoArg));

Ooh, we should fix that. The reinterpret_cast kind of defeats the purpose, since this is mainly for documentation. Would it work to just declare PoisonPtr as taking a void*, or do we need to templafuscate it?
Comment 5 Terrence Cole [:terrence] 2012-07-25 17:03:32 PDT
Making PoisonPtr take a void* worked like a charm:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6291bb72294
Comment 6 Ed Morley [:emorley] 2012-07-26 05:08:53 PDT
https://hg.mozilla.org/mozilla-central/rev/b6291bb72294

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