Bug 141871 - Throwing a frozen object makes it claim to be unfrozen
Summary: Throwing a frozen object makes it claim to be unfrozen
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.9
: P2 Normal
Assignee: Matthew Mirman
URL:
Keywords:
: 141878 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-02-21 16:45 PST by Mark S. Miller
Modified: 2015-04-09 08:49 PDT (History)
7 users (show)

See Also:


Attachments
patch. (52.46 KB, patch)
2015-03-31 14:30 PDT, Matthew Mirman
no flags Details | Formatted Diff | Diff
patch. (52.47 KB, patch)
2015-03-31 15:08 PDT, Matthew Mirman
ggaren: review-
ggaren: commit-queue-
Details | Formatted Diff | Diff
patch. (69.81 KB, patch)
2015-04-03 12:54 PDT, Matthew Mirman
ggaren: review-
ggaren: commit-queue-
Details | Formatted Diff | Diff
patch (76.03 KB, patch)
2015-04-06 16:21 PDT, Matthew Mirman
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark S. Miller 2015-02-21 16:45:47 PST
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.
Comment 1 Alexey Proskuryakov 2015-02-22 01:27:54 PST
Does this only occur on 7.0.5, or on current versions too? 7.0.5 is not even the newest major release.
Comment 2 Mark S. Miller 2015-02-22 09:37:13 PST
Just verified that the same symptoms occur on Version 7.1.3 (9537.85.12.18)
Comment 3 Matthew Mirman 2015-03-31 14:30:05 PDT
Created attachment 249851 [details]
patch.

Small benchmark on octane looked fine.  running a full benchmark now.
Comment 4 Filip Pizlo 2015-03-31 14:45:30 PDT
Can you rebase?
Comment 5 Matthew Mirman 2015-03-31 15:08:53 PDT
Created attachment 249858 [details]
patch.
Comment 6 Geoffrey Garen 2015-03-31 15:49:51 PDT
Comment on attachment 249858 [details]
patch.

EWS is failing.
Comment 7 Matthew Mirman 2015-04-03 12:54:31 PDT
Created attachment 250093 [details]
patch.

patch did not cause any performance regression on my machine.
Comment 8 Geoffrey Garen 2015-04-03 13:37:07 PDT
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?
Comment 9 Matthew Mirman 2015-04-03 15:02:26 PDT
(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.
Comment 10 Mark S. Miller 2015-04-03 15:13:42 PDT
Please also check that throwing a frozen Error does not cause a violation of the frozenness contract.
Comment 11 Matthew Mirman 2015-04-06 16:21:14 PDT
Created attachment 250238 [details]
patch
Comment 12 Matthew Mirman 2015-04-06 16:29:51 PDT
(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.
Comment 13 Matthew Mirman 2015-04-06 16:30:25 PDT
(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.
Comment 14 Geoffrey Garen 2015-04-07 14:03:46 PDT
Comment on attachment 250238 [details]
patch

r=me
Comment 15 Matthew Mirman 2015-04-07 14:36:54 PDT
Comment on attachment 250238 [details]
patch

landed: http://trac.webkit.org/changeset/182495
Comment 16 Matthew Mirman 2015-04-07 14:37:27 PDT
Landed in: http://trac.webkit.org/changeset/182495
Closing bug.
Comment 17 Geoffrey Garen 2015-04-08 13:31:11 PDT
*** Bug 141878 has been marked as a duplicate of this bug. ***
Comment 18 Brent Fulgham 2015-04-08 17:37:35 PDT
This change might have broken Web Inspector on Windows.

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