Last Comment Bug 572232 - js_InvokeConstructor with clampReturn==JS_TRUE and user-controlled ctor is a gc hazard
: js_InvokeConstructor with clampReturn==JS_TRUE and user-controlled ctor is a ...
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on: 567577
Blocks: jsfunfuzz 516827
  Show dependency treegraph
Reported: 2010-06-15 15:56 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2013-03-20 08:36 PDT (History)
15 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Branch patch (615 bytes, patch)
2010-07-27 17:18 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
christian: approval1.9.2.9+
christian: approval1.9.1.12+
Details | Diff | Splinter Review
Patch for 1.9.0 (1.51 KB, patch)
2010-09-03 09:45 PDT, Mike Hommey [:glandium]
dvander: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2010-06-15 15:56:34 PDT
See bug 567577 comment 9 and bug 567577 comment 10.

I *think* we either need to add a tvr for the noted default-return object in js_InvokeConstructor or figure out whether __noSuchMethod__ functionality can ever be invoked in a constructing situation.  The latter is probably hard, and the former is probably easy, which suggests probably the former is what we should do.

This also shows up in JS_New on trunk (but conservative stack scanning saves us) and in fun_applyConstructor (Narcissus-only, so I think we don't care), but neither of these cases needs a change.
Comment 1 Brendan Eich [:brendan] 2010-06-15 16:44:38 PDT
Cc'ing mrbkap who was involved in bug 453462, and gal who took over at some point.

Comment 2 Robert Sayre 2010-06-22 13:23:29 PDT
this is still biting on branches, even if we get a conservative scanner on trunk, right?
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2010-06-22 13:28:23 PDT
Comment 4 David Anderson [:dvander] 2010-06-29 13:02:15 PDT
The patch in bug 567577 fixes this, I think.
Comment 5 Robert Sayre 2010-06-29 13:12:02 PDT
(In reply to comment #4)
> The patch in bug 567577 fixes this, I think.

"Can we get a little more confidence than 'I Think'?" -- Damon Sicore
Comment 6 Andreas Gal :gal 2010-06-29 13:17:15 PDT
Waldo filed it. Waldo should decide. As far as I understand the bug report scanning the stack will find this root, so this is safe now. Waldo?
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2010-06-29 15:12:51 PDT
Comment 4 is correct with respect to trunk, even with conservative stack scanning turned off.  We may not want to apply the patch there to branches exactly as is, however, as it does force clamping, which would be a web-visible change.  Just adding the AutoObjectRooter (JSAutoTempValueRooter on branches) should be enough for branches, without compatibility concerns.
Comment 8 Robert Sayre 2010-07-13 13:38:43 PDT
Waldo, could you please get a branch patch going?
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-14 11:46:37 PDT
Comment 10 Damon Sicore (:damons) 2010-07-27 13:29:20 PDT
Any news on the branch patch?
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-27 17:18:12 PDT
Created attachment 460709 [details] [diff] [review]
Branch patch
Comment 12 christian 2010-08-02 15:44:36 PDT
Comment on attachment 460709 [details] [diff] [review]
Branch patch

a=LegNeato for and Please land on mozilla-1.9.2 default and mozilla-1.9.1 default.
Comment 14 Mike Hommey [:glandium] 2010-09-03 09:45:03 PDT
Created attachment 471885 [details] [diff] [review]
Patch for 1.9.0
Comment 15 Ed Morley [:emorley] 2013-03-20 04:48:57 PDT

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