Bug 143542 - [Win] Layout Test inspector-protocol/debugger/setPauseOnExceptions-all.html is failing
Summary: [Win] Layout Test inspector-protocol/debugger/setPauseOnExceptions-all.html i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matthew Mirman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-08 17:24 PDT by Brent Fulgham
Modified: 2015-04-09 17:01 PDT (History)
9 users (show)

See Also:


Attachments
patch. (7.86 KB, patch)
2015-04-09 15:57 PDT, Matthew Mirman
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-04-08 17:24:57 PDT
The following layout test is failing on [insert platform]

inspector-protocol/debugger/setPauseOnExceptions-all.html
inspector-protocol/debugger/setPauseOnExceptions-none.html
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
Comment 1 Radar WebKit Bug Importer 2015-04-08 17:25:31 PDT
<rdar://problem/20475871>
Comment 2 Brent Fulgham 2015-04-08 17:33:40 PDT
Tests marked as failing in r182575 <https://trac.webkit.org/changeset/182575>.
Comment 3 Alexey Proskuryakov 2015-04-08 21:32:34 PDT
This is a recent regression, correct?
Comment 4 Brent Fulgham 2015-04-08 21:33:39 PDT
(In reply to comment #3)
> This is a recent regression, correct?

Yes. I feel like we should roll out if the JSC team can't fix it.
Comment 5 Brent Fulgham 2015-04-09 08:49:27 PDT
This appears to have been introduced by Bug 141871. <http://trac.webkit.org/changeset/182495>
Comment 6 Matthew Mirman 2015-04-09 15:11:06 PDT
The tests expectations are also wrong when run manually on osx on my machine. I suspect they aren't getting run on osx bots but I wouldn't know why.  

This isn't necessarily a bug in the code, and expectations need to be updated.  Specifically, because "sourceURL" is now being appended at error construction time rather than when thrown, the tests now attempt to append it during the code run by "InspectorTest.sendCommand" which I assume uses an eval to run it.  Because this code is generated on the fly, it has no URL and thus sourceURL makes no sense.   Also since they are generated in a new locations, they will have a new lines and "columns.
Comment 7 Matthew Mirman 2015-04-09 15:57:30 PDT
Created attachment 250480 [details]
patch.
Comment 8 Michael Saboff 2015-04-09 16:17:16 PDT
Comment on attachment 250480 [details]
patch.

r=me
Comment 9 Joseph Pecoraro 2015-04-09 16:22:19 PDT
(In reply to comment #6)
> The tests expectations are also wrong when run manually on osx on my
> machine. I suspect they aren't getting run on osx bots but I wouldn't know
> why.

They are probably skipped on Mac ports. Inspector tests are pretty much all flakey. Something that will be looked at soon.
Comment 10 Joseph Pecoraro 2015-04-09 16:25:26 PDT
Comment on attachment 250480 [details]
patch.

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

> LayoutTests/ChangeLog:3
> +        Updated expectations on a few windows tests to account for error

These are not "windows tests". They are inspector tests.

> LayoutTests/ChangeLog:10
> +        Because sourceURL is now being appended at error construction time rather than when thrown, 

Why did we change to error construction time? I find it less useful, at least in this situation.

In this case, wouldn't it be more useful to know the `throw` happened at "exception.js:18" then the call site?
Comment 11 Matthew Mirman 2015-04-09 16:44:23 PDT
Comment on attachment 250480 [details]
patch.

Patch landed.  http://trac.webkit.org/changeset/182614
Comment 12 Matthew Mirman 2015-04-09 16:44:46 PDT
Closing bug. http://trac.webkit.org/changeset/182614
Comment 13 Matthew Mirman 2015-04-09 17:01:49 PDT
Comment on attachment 250480 [details]
patch.

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

>> LayoutTests/ChangeLog:3
>> +        Updated expectations on a few windows tests to account for error
> 
> These are not "windows tests". They are inspector tests.

Fixed in submission.

>> LayoutTests/ChangeLog:10
>> +        Because sourceURL is now being appended at error construction time rather than when thrown, 
> 
> Why did we change to error construction time? I find it less useful, at least in this situation.
> 
> In this case, wouldn't it be more useful to know the `throw` happened at "exception.js:18" then the call site?

A. Adding/modifying the object being thrown goes against the ecma standard.  For example, suppose somebody were to throw an object as part of some strange control flow and then check that line/column/stack/sourceURL weren't present, or want to add their own stack, or enumerate over properties of the object (most likely) and expect those to be empty.  
B. Other browsers don't do it, so we really shouldn't have to.
C. Moving it fixes the problem where frozen objects can get unfrozen.  This isn't the only solution, but in light of the above, it feels like the correct solution. 
D. Slows down code that uses throw/catch as a control construct (which people do and have done despite it not being good style and being currently inefficient).

Rather than reporting where a throw happened, the inspector inspector will reports where a native error was constructed, which really makes a lot of sense.  

Note, there is a potential future way to bring back *some* of the old semantics:  throw can check if an object has an "error info setter" and call it if it does (something like __appendThrowInfo__).  
If it does, when the object is thrown, this method will be called and can append the new stack and source info.  It also has the advantage of being customizable.