1128648 - Remove fretting about DynamicWithObject in Shape::set

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Old old old old code, as indicated by the comment's reference to "it", a demo object that was removed from the shell years ago. A lot has changed since.

The fact that the obj argument can simply be changed from HandleObject to HandleNativeObject, without other changes, explains why it's safe to remove the old special case. WithObjects aren't native.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: MozReview Request: bz://1128648/jorendorff

/r/3263 - Bug 1128648 - Remove fretting about DynamicWithObject in Shape::set. r??efaust.

Pull down this commit:

hg pull review -r b04e74d1ddaa3a7f86655a04e7302d87e1befeb6

Comment 2

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=8dbf157c35ff

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558086 [details]
MozReview Request: bz://1128648/jorendorff

/r/3263 - Bug 1128648 - Remove fretting about DynamicWithObject in Shape::set. r?efaust.

Pull down this commit:

hg pull review -r 3c1ac8c9c29755a42ad2a549fe9e3635ad003b48

Comment 4

[Jason Orendorff [:jorendorff]]

<evilpie> jorendorff: you wish! nobody bothered to set non_native on with yet

Tom's right. I filed bug 1128681 to give a permanent home to this outrage, and added an assertion in Shape::set. The shell tests do not trip the assertion; we'll see what a try run turns up.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558086 [details]
MozReview Request: bz://1128648/jorendorff

/r/3263 - Bug 1128648 - Remove fretting about DynamicWithObject in Shape::set. r?efaust.

Pull down this commit:

hg pull review -r 3c1ac8c9c29755a42ad2a549fe9e3635ad003b48

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8558086 [details]
MozReview Request: bz://1128648/jorendorff

https://reviewboard.mozilla.org/r/3261/#review2685

LGTM. We will want to make sure we get the WithObject stuff cleaned up, and ensure that they stop flowing through Shape::set if that's unsafe.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558086 [details]
MozReview Request: bz://1128648/jorendorff

/r/3263 - Bug 1128648 - Remove fretting about DynamicWithObject in Shape::set. r=efaust.
/r/3311 - Bug 1128732 - Simplify js::DefineProperties. r?efaust.
/r/3313 - Bug 1129275 - Remove extra js::SetProperty template. r?efaust.

Pull down these commits:

hg pull review -r 12796fabc54b3f83be57b22546420c36737b85a8

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558086 [details]
MozReview Request: bz://1128648/jorendorff

Wat. I didn't mean to attach anything to this bug. I pushed a different patch (unrelated to this bug) for review. Setting the flag back to r+.

Comment 9

[Eric Faust [:efaust]]

https://reviewboard.mozilla.org/r/3263/#review2717

Looks good again.

::: js/src/vm/Shape-inl.h
(Diff revision 3)
> +    MOZ_ASSERT(!obj->is<DynamicWithObject>());  // See bug 1128681.

This makes me feel much better about things.

Comment 10

[Eric Faust [:efaust]]

https://reviewboard.mozilla.org/r/3313/#review2719

Yah.

Comment 11

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4232dc24dc1

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: MozReview Request: Bug 1129275 - Remove extra js::SetProperty template. r?efaust.

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: MozReview Request: Bug 1128732 - Simplify js::DefineProperties. r?efaust.

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: MozReview Request: Bug 1128648 - Remove fretting about DynamicWithObject in Shape::set. r=efaust.