Last Comment Bug 737640 - BaseShape default copy constructor gets used
: BaseShape default copy constructor gets used
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Terrence Cole [:terrence]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-20 14:16 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-05-15 06:33 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Define a copy constructor for BaseShape. v1 (2.34 KB, patch)
2012-03-20 14:31 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
v1: Go through StackBaseShape. (1.80 KB, patch)
2012-05-14 15:04 PDT, Terrence Cole [:terrence]
bhackett1024: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-03-20 14:16:30 PDT
This confused the crap out of me me while debugging. Patch coming right up.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-03-20 14:31:20 PDT
Created attachment 607723 [details] [diff] [review]
Define a copy constructor for BaseShape. v1

Attaching a patch - flagging bhackett for review.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-03-20 14:32:19 PDT
CCing billm, because I think this might have been a GC hazard.

Bill, judging from what happens in the other constructors, it looks like we weren't doing write barriers correctly before in the copy constructor case. Do we need write barriers for things that were previously null?
Comment 3 Andrew McCreight [:mccr8] 2012-03-20 14:34:54 PDT
(In reply to Bobby Holley (:bholley) from comment #2)
> Do we need write barriers for things that were previously null?

Not for incremental GC, but we will need them for generational, I believe.
Comment 4 Bill McCloskey (:billm) 2012-03-20 14:50:48 PDT
(In reply to Andrew McCreight [:mccr8] from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > Do we need write barriers for things that were previously null?
> 
> Not for incremental GC, but we will need them for generational, I believe.

Yeah. Given a HeapValue or HeapPtr, you can use the .init() method rather than assignment, and this will skip the incremental write barrier. It will still do the generational barrier.
Comment 5 Brian Hackett (:bhackett) 2012-03-21 05:02:58 PDT
Comment on attachment 607723 [details] [diff] [review]
Define a copy constructor for BaseShape. v1

The copy constructor for BaseShape should be MOZ_DELETE.  Where is it actually used?
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-03-21 10:16:37 PDT
(In reply to Brian Hackett (:bhackett) from comment #5)

> Where is it actually used?

http://mxr.mozilla.org/mozilla-central/source/js/src/jsscope.cpp#118
Comment 7 Terrence Cole [:terrence] 2012-05-14 15:04:03 PDT
Created attachment 623841 [details] [diff] [review]
v1: Go through StackBaseShape.
Comment 8 Terrence Cole [:terrence] 2012-05-14 16:33:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf06b5b0a96b
Comment 9 Ed Morley [:emorley] 2012-05-15 06:33:18 PDT
https://hg.mozilla.org/mozilla-central/rev/bf06b5b0a96b

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