Bug 141871 – Throwing a frozen object makes it claim to be unfrozen

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark S. Miller]

This occurs on Safari 7.0.5 (9537.77.4). Discovered by Kevin Reid.

Test case:

var o = Object.freeze([1, 2]); try { console.log(Object.isFrozen(o)); throw o; } catch (e) { console.log(e.toString(), Object.isFrozen(e), e === o); }

Output in Safari;
[Log] true
[Log] 1,2 false true



Throwing o made the subsequent isFrozen claim that it isn't. But as far as we can tell so far by attempting to mutate it, it actually is so anyway.

[Alexey Proskuryakov]

Does this only occur on 7.0.5, or on current versions too? 7.0.5 is not even the newest major release.

[Mark S. Miller]

Just verified that the same symptoms occur on Version 7.1.3 (9537.85.12.18)

[Matthew Mirman]

Created attachment 249851 [details]
patch.

Small benchmark on octane looked fine.  running a full benchmark now.

[Filip Pizlo]

Can you rebase?

[Matthew Mirman]

Created attachment 249858 [details]
patch.

[Geoffrey Garen]

Comment on attachment 249858 [details]
patch.

EWS is failing.

[Matthew Mirman]

Created attachment 250093 [details]
patch.

patch did not cause any performance regression on my machine.

[Geoffrey Garen]

Comment on attachment 250093 [details]
patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=250093&action=review

Which part of this patch removes error info that we used to add?

You need to add a test for that part of the patch, both with frozen and unfrozen objects, and preferably with various types of objects, including DOM nodes, vanilla objects, arrays, and so on.

> LayoutTests/ChangeLog:3
> +        Source and stack information should get appended only to native error constructors

I think you meant "native errors" and not "native error constructors". It's the error that holds the information, not the error's constructor.

> LayoutTests/ChangeLog:4
> +        Source and stack information should get appended only to native error constructors
> +        and should be added at construction time rather than when thrown.

Why did you make this change? You should explain why this new behavior is preferable to the old behavior.

> LayoutTests/http/tests/w3c/resources/testharness.js:2070
>      function AssertionError(message)
>      {
> +        var e = new Error();
> +        this.stack = e.stack.substring(e.stack.indexOf('\n')+1, e.stack.length);
>          this.message = message;
>      }

It looks like this test was imported from the W3C, but it required modification in order to continue passing after your code changes. Doest this mean that the W3C test, when run from source, no longer passes? Does it pass in other browsers?

> LayoutTests/js/dom/stack-trace-expected.txt:359
> -    0   inlineableThrow at stack-trace.js:302:33
> +    0   inlineableThrow at stack-trace.js:302:40

Why did this result change? I think I might know, but I'm not sure. This is the kind of thing you should explain in your ChangeLog.

> LayoutTests/js/exception-properties-expected.txt:6
> -PASS enumerableProperties(error) is []
> +PASS enumerableProperties(error).sort() is ["column", "line", "sourceURL"]

Why is it good that these properties are enumerable now?

> Source/JavaScriptCore/ChangeLog:4
> +        Source and stack information should get appended only to native error constructors
> +        and should be added at construction time rather than when thrown.

Same comment from other ChangeLog.

> Source/JavaScriptCore/parser/ParserError.h:97
>              return addErrorInfo(

If we're only supposed to add error properties during construction, why do we still have an addErroInfo helper function, which adds those properties after construction?

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:40
> +static void appendSourceToError(CallFrame* callFrame, ErrorInstance* exception, unsigned bytecodeOffset)

Did any of this code change, or did you just move it? If you just moved it, why did you move it? This is the kind of thing you should explain in your ChangeLog.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:219
> +    addErrorInfo(exec, asObject(errorObject), true);

Why do we need to do this extra step after error construction? Can the error constructor do this step for us?

[Matthew Mirman]

(In reply to comment #8)
> Comment on attachment 250093 [details]
> patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250093&action=review
> 
> Which part of this patch removes error info that we used to add?

It doesn't remove the error info entirely, it just now adds it eagerly at error construction for native errors.  The change that removes it from being added to all "throw"  is in VM.cpp

> You need to add a test for that part of the patch, both with frozen and
> unfrozen objects, and preferably with various types of objects, including
> DOM nodes, vanilla objects, arrays, and so on.
> 
sure

> > LayoutTests/ChangeLog:3
> > +        Source and stack information should get appended only to native error constructors
> 
> I think you meant "native errors" and not "native error constructors". It's
> the error that holds the information, not the error's constructor.

yes.
 
> > LayoutTests/ChangeLog:4
> > +        Source and stack information should get appended only to native error constructors
> > +        and should be added at construction time rather than when thrown.
> 
> Why did you make this change? You should explain why this new behavior is
> preferable to the old behavior.
> 
> > LayoutTests/http/tests/w3c/resources/testharness.js:2070
> >      function AssertionError(message)
> >      {
> > +        var e = new Error();
> > +        this.stack = e.stack.substring(e.stack.indexOf('\n')+1, e.stack.length);
> >          this.message = message;
> >      }
> 
> It looks like this test was imported from the W3C, but it required
> modification in order to continue passing after your code changes. Doest
> this mean that the W3C test, when run from source, no longer passes? Does it
> pass in other browsers?

the test passes in other browsers, and if our test which used the native W3C test passed, it would also pass, but it doesn't, and our tests currently expect a failure with a stack trace, which other browsers simply don't append.

> > LayoutTests/js/dom/stack-trace-expected.txt:359
> > -    0   inlineableThrow at stack-trace.js:302:33
> > +    0   inlineableThrow at stack-trace.js:302:40
> 
> Why did this result change? I think I might know, but I'm not sure. This is
> the kind of thing you should explain in your ChangeLog.

I changed a few things along the lines of "throw {}" to "throw new Error()"  which changed the precise location of the throw.

[Mark S. Miller]

Please also check that throwing a frozen Error does not cause a violation of the frozenness contract.

[Matthew Mirman]

Created attachment 250238 [details]
patch

[Matthew Mirman]

(In reply to comment #8)
> > LayoutTests/js/exception-properties-expected.txt:6
> > -PASS enumerableProperties(error) is []
> > +PASS enumerableProperties(error).sort() is ["column", "line", "sourceURL"]
> 
> Why is it good that these properties are enumerable now?

Its not really either good nor bad.  Throw shouldn't be modifying the object being thrown at all, and native errors were expected to have enumerable properties after being thrown. Now since those properties are added before they are thrown, it is reasonable to assume that those properties should be enumerable before being thrown.

> > Source/JavaScriptCore/ChangeLog:4
> > +        Source and stack information should get appended only to native error constructors
> > +        and should be added at construction time rather than when thrown.
> 
> Same comment from other ChangeLog.

Fixed.

> > Source/JavaScriptCore/parser/ParserError.h:97
> >              return addErrorInfo(
> 
> If we're only supposed to add error properties during construction, why do
> we still have an addErroInfo helper function, which adds those properties
> after construction?

Because WebCore adds "native" errors which do not inherit from any JSC native error, appending the error properties as a seperate call after construction of the error is required to avoid having to manually truncate the stack and gather local source information due to the stack being extended by a nested call to construct one of the native jsc error.

> > Source/JavaScriptCore/runtime/ErrorInstance.cpp:40
> > +static void appendSourceToError(CallFrame* callFrame, ErrorInstance* exception, unsigned bytecodeOffset)
> 
> Did any of this code change, or did you just move it? If you just moved it,
> why did you move it? This is the kind of thing you should explain in your
> ChangeLog.

nope, it moved since it was needed elsewhere and wasn't in the global namespace.

> > Source/WebCore/bindings/js/JSDOMBinding.cpp:219
> > +    addErrorInfo(exec, asObject(errorObject), true);
> 
> Why do we need to do this extra step after error construction? Can the error
> constructor do this step for us?

In the JSC native error, yes, we could make this call in the error constructor and in fact we do.  However, WebCore also has its own native error, and making these inherit from JSC's native errors would make the stack include an extra layer of 
indirection.  Furthermore, there isn't just one native webcore error, there are quite a few with macro generated constructors.  Calling it once just before the error is thrown in webcore is the simplest way of adding this information.

[Matthew Mirman]

(In reply to comment #10)
> Please also check that throwing a frozen Error does not cause a violation of
> the frozenness contract.

fixed in the current patch.

[Geoffrey Garen]

Comment on attachment 250238 [details]
patch

r=me

[Matthew Mirman]

Comment on attachment 250238 [details]
patch

landed: http://trac.webkit.org/changeset/182495

[Matthew Mirman]

Landed in: http://trac.webkit.org/changeset/182495
Closing bug.

[Geoffrey Garen]

*** Bug 141878 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

This change might have broken Web Inspector on Windows.

See <https://bugs.webkit.org/show_bug.cgi?id=143542>.