Bug 143542 – [Win] Layout Test inspector-protocol/debugger/setPauseOnExceptions-all.html is failing

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brent Fulgham]

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

[Radar WebKit Bug Importer]

<rdar://problem/20475871>

[Brent Fulgham]

Tests marked as failing in r182575 <https://trac.webkit.org/changeset/182575>.

[Alexey Proskuryakov]

This is a recent regression, correct?

[Brent Fulgham]

(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.

[Brent Fulgham]

This appears to have been introduced by Bug 141871. <http://trac.webkit.org/changeset/182495>

[Matthew Mirman]

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.

[Matthew Mirman]

Created attachment 250480 [details]
patch.

[Michael Saboff]

Comment on attachment 250480 [details]
patch.

r=me

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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?

[Matthew Mirman]

Comment on attachment 250480 [details]
patch.

Patch landed.  http://trac.webkit.org/changeset/182614

[Matthew Mirman]

Closing bug. http://trac.webkit.org/changeset/182614

[Matthew Mirman]

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.