Last Comment Bug 707842 - Multiple shapes are sharing BaseShapes that have PropertyTables, which shouldn't happen
: Multiple shapes are sharing BaseShapes that have PropertyTables, which should...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: DMD
Blocks: ObjectShrink
  Show dependency treegraph
 
Reported: 2011-12-05 17:46 PST by Nicholas Nethercote [:njn]
Modified: 2011-12-10 20:45 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
improve Shape constructors (8.45 KB, patch)
2011-12-07 11:14 PST, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-12-05 17:46:34 PST
DMD tells me that PropertyTables are being counted twice.  The attached file contains the error message.  There's only one place that PropertyTables are counted (the shape->sizeOfPropertyTable() call in XPCJSRuntime.cpp), so the problem doesn't appear to be with the memory reporting code.  

bhackett said on IRC: "base shapes with property tables are supposed to be 1:1 with the shapes that own them, but if multiple shapes have the same owned base shape then the double counting could happen".  So it's probably a problem with the handling of Shapes and BaseShapes.
Comment 1 Brian Hackett (:bhackett) 2011-12-07 11:14:30 PST
Created attachment 579762 [details] [diff] [review]
improve Shape constructors

Improve the types of the shape constructors to require that an unowned base shape be initially supplied.  Doing this caught one instance we were passing a possibly owned base shape, in JSObject::addPropertyInternal, though I haven't confirmed this is definitely the cause of this bug.
Comment 2 Luke Wagner [:luke] 2011-12-08 16:32:30 PST
Comment on attachment 579762 [details] [diff] [review]
improve Shape constructors

Mmm types.
Comment 3 Brian Hackett (:bhackett) 2011-12-08 19:37:51 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a742eac3ab
Comment 4 Nicholas Nethercote [:njn] 2011-12-08 21:53:09 PST
I just did a couple of DMD runs and I'm not getting the warnings about double-counting any more!  Good news.

bhackett, could bad things have happened as a result of this bug?
Comment 5 Brian Hackett (:bhackett) 2011-12-09 07:18:41 PST
(In reply to Nicholas Nethercote [:njn] from comment #4)
> I just did a couple of DMD runs and I'm not getting the warnings about
> double-counting any more!  Good news.
> 
> bhackett, could bad things have happened as a result of this bug?

Sharing an owned base shape between multiple shapes could cause incorrect lookups --- one or the other shape will have an incorrect property table, and when doing a search for a native property of the object a shape might not be found even though it is on the object.  I *think* that is the only fallout of this particular problem.
Comment 6 Ed Morley [:emorley] 2011-12-10 20:45:32 PST
https://hg.mozilla.org/mozilla-central/rev/a4a742eac3ab

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