Last Comment Bug 311022 - Greasemonkey 0.6.2 exposes window wrapper proliferation when enumerating
: Greasemonkey 0.6.2 exposes window wrapper proliferation when enumerating
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla1.8beta5
Assigned To: Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017)
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 310864
  Show dependency treegraph
 
Reported: 2005-10-04 00:39 PDT by Brendan Eich [:brendan]
Modified: 2005-10-04 23:36 PDT (History)
7 users (show)
brendan: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't make lots of wrappers (1.44 KB, patch)
2005-10-04 15:28 PDT, Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017)
no flags Details | Diff | Splinter Review
Move the test to where it's needed (1.39 KB, patch)
2005-10-04 15:34 PDT, Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017)
bzbarsky: review+
jst: superreview+
brendan: approval1.8b5+
Details | Diff | Splinter Review

Description Brendan Eich [:brendan] 2005-10-04 00:39:29 PDT
This is probably a split window bug exposed by the fix for bug 308856.  We see
stacks overflowing via MarkSharpObjects, starting with a window.arguments Array,
descending through 0 to a window object, through its 'window' property to a new
and different flat JSObject for an XPCNativeWrapper for the same native identity
pointer, etc. etc.

Somewhere under nsWindowSH::GetProperty we are rewrapping when we should be
finding a native wrapper.

Or perhaps we're doing some outer=>inner=>outer dance that causes the same result.

/be
Comment 1 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2005-10-04 15:28:35 PDT
Created attachment 198513 [details] [diff] [review]
Don't make lots of wrappers

bz pointed me at this on IRC. We can't fall into the GetNewOrUsed case because
that always returns an implicit wrapper. This patch prevents us from creating
lots of new wrappers for the same object, and simply returns the one that we've
already created.
Comment 2 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2005-10-04 15:34:31 PDT
Created attachment 198514 [details] [diff] [review]
Move the test to where it's needed

The other patch was an old one, this one is current.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-04 15:39:03 PDT
Comment on attachment 198514 [details] [diff] [review]
Move the test to where it's needed

sr=jst
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2005-10-04 15:40:36 PDT
Comment on attachment 198514 [details] [diff] [review]
Move the test to where it's needed

r=bzbarsky.  This should probably go in on branch too -- it's a very safe fix
and makes greasemonkey a lot happier.
Comment 5 Brendan Eich [:brendan] 2005-10-04 15:53:17 PDT
Comment on attachment 198514 [details] [diff] [review]
Move the test to where it's needed

a=me, we have high confidence in spite of high regression rate overall.  This
code is better than the average, and the patch is only gonna make the bad case
go away.

/be
Comment 6 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2005-10-04 16:00:40 PDT
Fix checked in, MOZILLA_1_8_BRANCH and trunk.

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