Closed Bug 356085 Opened 14 years ago Closed 14 years ago

Incorrect uneval of object with named getter function; property and function names are mashed together

Categories

(Core :: JavaScript Engine, defect, minor)

1.8 Branch
defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: jruderman, Assigned: crowderbt)

References

Details

(Keywords: testcase, verified1.8.1.1)

Attachments

(1 file, 1 obsolete file)

js> uneval({p setter: function y() { } })               
({set py() {}})
Assignee: general → crowder
Status: NEW → ASSIGNED
This isn't a decompiler bug, but a bug in js_obj_toString.  I believe this is the correct fix.
Attachment #241792 - Flags: review?(brendan)
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Comment on attachment 241792 [details] [diff] [review]
Fixing js_obj_toString for getter/setter

>             if (!gsop[j])
>                 chars[nchars++] = ':';
>+            else
>+                chars[nchars++] = ' ';

If this were the fix, it should be minimized as

              chars[nchars++] = gsop[j] ? ' ' : ':';

But is it the right fix?  With the input in comment 0, what output do you now get?

/be
Comment on attachment 241792 [details] [diff] [review]
Fixing js_obj_toString for getter/setter

Ah, yes -- rogerl coupled the decompiler to string padding details in js_obj_toSource.  I see the fix and I'll r+ a minimized patch.  Thanks,

/be
Semi-relatedly, I looked around only briefly, but this code seems to treat the "chars" buffer as if it will magically grow without regard to what is being shoved into it.  Is it safe just because the buffer is assumed to be Way Too Big for trouble to happen?
Attachment #241792 - Attachment is obsolete: true
Attachment #241796 - Flags: review?(brendan)
Attachment #241792 - Flags: review?(brendan)
(In reply to comment #4)
> Created an attachment (id=241796) [edit]
> more terse version
> 
> Semi-relatedly, I looked around only briefly, but this code seems to treat the
> "chars" buffer as if it will magically grow without regard to what is being
> shoved into it.  Is it safe just because the buffer is assumed to be Way Too
> Big for trouble to happen?

No.  Read up to around line 976:

            curlen = nchars;
            if (comma)
                SAFE_ADD(2);
            SAFE_ADD(idstrlength + 1);
            if (gsop[j])
                SAFE_ADD(JSSTRING_LENGTH(gsop[j]) + 1);
            SAFE_ADD(vsharplength);
            SAFE_ADD(vlength);
            /* Account for the trailing null. */
            SAFE_ADD((outermost ? 2 : 1) + 1);
#undef SAFE_ADD

            if (curlen > (size_t)-1 / sizeof(jschar))
                goto overflow;

            /* Allocate 1 + 1 at end for closing brace and terminating 0. */
            chars = (jschar *)
                realloc((ochars = chars), curlen * sizeof(jschar));

Notice that the space for the : is included along with idstrlength.

/be
Comment on attachment 241796 [details] [diff] [review]
more terse version

r=me, thanks.  Good for 1.8.1.1/js1.7src.

/be
Attachment #241796 - Flags: review?(brendan) → review+
Blocks: js1.7src
Flags: blocking1.8.1.1?
Whiteboard: [checkin needed]
mozilla/js/src/jsobj.c 	3.296
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Function decompilation has the same bug, which I filed as bug 356248.

What the fuzzer found was actually that bug.  I thought I was "reducing" the testcase by taking the object out of the function, not realizing that object uneval and decompilation of object literals within functions use separate code.
No longer blocks: 349611
Summary: Incorrect decompilation for named getter function; property and function names are mashed together → Incorrect uneval of object with named getter function; property and function names are mashed together
Checking in regress-356085.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-356085.js,v  <--  regress-356085.js
initial revision: 1.1
done

Note that this stills show the assert in bug 356250 until it is fixed.
Flags: in-testsuite+
I no longer get the assert in 20061014 builds bug the test needs to be modified to expect the value : ' ( { set p y ( ) { } } ) '

Checking in regress-356085.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-356085.js,v  <--  regress-356085.js
new revision: 1.2; previous revision: 1.1
done

verified fixed 1.9 20061014 windows/linux
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment on attachment 241796 [details] [diff] [review]
more terse version

applies and builds cleanly for me on branch
Attachment #241796 - Flags: approval1.8.1.1?
Comment on attachment 241796 [details] [diff] [review]
more terse version

Approved for 1.8.1 branch, a=jay for drivers.  Please land this asap, thanks!
Attachment #241796 - Flags: approval1.8.1.1? → approval1.8.1.1+
mozilla/js/src/jsobj.c 	3.208.2.42
Keywords: fixed1.8.1.1
Target Milestone: --- → mozilla1.8.1
Version: Trunk → 1.8 Branch
verified fixed 20061123 1.8.1.1 windows/linux/mac*, 1.9 windows/linux
the decompilation has changed. Does this matter?
expect: ( { set p y ( ) { } } )
actual: ( { set p ( ) { } } ) 
This should be fixed now, btw.
You need to log in before you can comment on or make changes to this bug.