507180 - imacros produce incorrect results with non-primitive valueOf

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Luke Wagner [:luke]]

Attachment: JSOP_ADD in JIT vs. interp

All three JSOP_ADD imacros produce the wrong result when there is a valueOf property and the valueOf property returns a non-primitive.  It calls toString on the return of valueOf instead of the object.  The attachment shows the variation.

Comment 1

[Luke Wagner [:luke]]

Attachment: fix in imacros.jsasm

Comment 2

[Luke Wagner [:luke]]

Although I'd need to read the spec more to be sure, it appears that this error is in most imacros.

Comment 3

[Brendan Eich [:brendan]]

Waldo, didn't you file a bug like this a while ago?

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 391373 [details] [diff] [review]
fix in imacros.jsasm

Good catch, patch looks correct but I'm short on sleep, so I'll let Waldo do the deed.

/be

Comment 5

[Brendan Eich [:brendan]]

Basic language semantics correctness issue, could bite in the wild but we don't know yet.

/be

Comment 6

[Luke Wagner [:luke]]

Attachment: same fix applied to all imacros with the same bug pattern

The fix is just adding "pop;dup" so that toString is called on the object, not the return value of valueOf.

Comment 7

[Jeff Walden [:Waldo]]

Would you add scads of trace-tests for these cases to the patch, please?

Comment 8

[Luke Wagner [:luke]]

In producing said scads, I notice that, as far as I can tell, equality.obj_obj cannot be invoked; it means "same object" and requires no conversion.  Incidentally, equality_imacros.obj_obj isn't referenced in jstracer.  Should I remove it?

Comment 9

[Luke Wagner [:luke]]

Attachment: all the changes

the last hg diff left out some changes... somehow

Comment 10

[Luke Wagner [:luke]]

Attachment: Regression test for all affected imacros

This test exercises all affected imacros.  It needs to be tweaked by someone test-savvy to fit into the js test harness.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #8)
> In producing said scads, I notice that, as far as I can tell, equality.obj_obj
> cannot be invoked; it means "same object" and requires no conversion. 

I see only equality_imacros.any_obj and .obj_any. What rev are you looking at?

/be

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #11)
> (In reply to comment #8)
> > In producing said scads, I notice that, as far as I can tell, equality.obj_obj
> > cannot be invoked; it means "same object" and requires no conversion. 
> 
> I see only equality_imacros.any_obj and .obj_any. What rev are you looking at?

Oops, I was looking at the wrong imacro.  Or maybe I went back in time and removed equality.obj_obj.  You never know...

Comment 13

[Daniel Veditz [:dveditz]]

Why is this nominated as a "blocker"? We'll approve correctness fixes as they get fixed, reviewed and request branch approval. Unless this is a regression from a previous security fix? We'd prioritize that a little higher.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 392523 [details] [diff] [review]
all the changes

Egad, this is an awful lot of copy-paste errors...want self-hosting (with name-freezing up the wazoo to avoid property/function guarding, of course)!

Comment 15

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/a10a85319c3b

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a10a85319c3b

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d1cbe0aa3915