572232 - js_InvokeConstructor with clampReturn==JS_TRUE and user-controlled ctor is a gc hazard

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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]]

Cc'ing mrbkap who was involved in bug 453462, and gal who took over at some point.

/be

Comment 2

[Robert Sayre]

this is still biting on branches, even if we get a conservative scanner on trunk, right?

Comment 3

[Jeff Walden [:Waldo]]

Yes.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

The patch in bug 567577 fixes this, I think.

Comment 5

[Robert Sayre]

(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]

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]]

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]

Waldo, could you please get a branch patch going?

Comment 9

[Jeff Walden [:Waldo]]

Sure.

Comment 10

[Damon Sicore (:damons)]

Any news on the branch patch?

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Branch patch

Comment 12

[LegNeato]

Comment on attachment 460709 [details] [diff] [review]
Branch patch

a=LegNeato for 1.9.2.9 and 1.9.1.12. Please land on mozilla-1.9.2 default and mozilla-1.9.1 default.

Comment 13

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c54b6c62214d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/95e8deb0774b

Comment 14

[Mike Hommey [:glandium]]

Attachment: Patch for 1.9.0

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/edd1edc19f46