Bug 171776 - Introduce ExceptionScope::assertNoException() and releaseAssertNoException().
Summary: Introduce ExceptionScope::assertNoException() and releaseAssertNoException().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-06 15:14 PDT by Mark Lam
Modified: 2017-05-08 12:53 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch. (83.28 KB, patch)
2017-05-06 15:50 PDT, Mark Lam
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-05-06 15:14:34 PDT
Instead of ASSERT(!scope.exception()), we can now do scope.assertNoException().  Ditto for RELEASE_ASSERT and scope.releaseAssertNoException().  

The advantage of using ExceptionScope::assertNoException() and releaseAssertNoException() is that if the assertion fails, these utility functions will print the stack trace for where the unexpected exception is detected as well as where the unexpected exception was thrown from.  This makes it much easier to debug the source of unhandled exceptions.
Comment 1 Mark Lam 2017-05-06 15:50:51 PDT
Created attachment 309300 [details]
proposed patch.

Let's try this on the EWS.
Comment 2 Build Bot 2017-05-06 16:35:57 PDT
Comment on attachment 309300 [details]
proposed patch.

Attachment 309300 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3689922

New failing tests:
ChakraCore.yaml/ChakraCore/test/Strings/HTMLHelpers.js.default
Comment 3 Mark Lam 2017-05-06 19:54:20 PDT
(In reply to Build Bot from comment #2)
> Comment on attachment 309300 [details]
> proposed patch.
> 
> Attachment 309300 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3689922
> 
> New failing tests:
> ChakraCore.yaml/ChakraCore/test/Strings/HTMLHelpers.js.default

I can't reproduce this locally.  Will rebase and try again.
Comment 4 Mark Lam 2017-05-06 20:28:14 PDT
(In reply to Mark Lam from comment #3)
> (In reply to Build Bot from comment #2)
> > Comment on attachment 309300 [details]
> > proposed patch.
> > 
> > Attachment 309300 [details] did not pass jsc-ews (mac):
> > Output: http://webkit-queues.webkit.org/results/3689922
> > 
> > New failing tests:
> > ChakraCore.yaml/ChakraCore/test/Strings/HTMLHelpers.js.default
> 
> I can't reproduce this locally.  Will rebase and try again.

Nope, not bogus.  I can reproduce it now after I rebased to ToT.  Probably not due to my patch, but I'll investigate.
Comment 5 Mark Lam 2017-05-06 21:25:43 PDT
(In reply to Mark Lam from comment #4)
> (In reply to Mark Lam from comment #3)
> > (In reply to Build Bot from comment #2)
> > > Comment on attachment 309300 [details]
> > > proposed patch.
> > > 
> > > Attachment 309300 [details] did not pass jsc-ews (mac):
> > > Output: http://webkit-queues.webkit.org/results/3689922
> > > 
> > > New failing tests:
> > > ChakraCore.yaml/ChakraCore/test/Strings/HTMLHelpers.js.default
> > 
> > I can't reproduce this locally.  Will rebase and try again.
> 
> Nope, not bogus.  I can reproduce it now after I rebased to ToT.  Probably
> not due to my patch, but I'll investigate.

OK, I found out that:
1. the issue reproduces without my patch.
2. the issue is intermittent and doesn't always reproduce.

Given that, I don't think it should block this patch.  Let's get a review.
Comment 6 Mark Lam 2017-05-06 21:34:52 PDT
(In reply to Mark Lam from comment #5)
> > Nope, not bogus.  I can reproduce it now after I rebased to ToT.  Probably
> > not due to my patch, but I'll investigate.
> 
> OK, I found out that:
> 1. the issue reproduces without my patch.
> 2. the issue is intermittent and doesn't always reproduce.

FYI, I filed https://bugs.webkit.org/show_bug.cgi?id=171786 to track the ChakraCore test failure bug.
Comment 7 Keith Miller 2017-05-08 09:38:33 PDT
Comment on attachment 309300 [details]
proposed patch.

r=me.
Comment 8 Mark Lam 2017-05-08 09:57:30 PDT
Thanks for the review.  Landed in r216428: <http://trac.webkit.org/r216428>.
Comment 9 Ryan Haddad 2017-05-08 12:28:01 PDT
(In reply to Mark Lam from comment #8)
> Thanks for the review.  Landed in r216428: <http://trac.webkit.org/r216428>.

It appears that this change broke the Windows Debug build: https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/1120
Comment 10 Mark Lam 2017-05-08 12:51:28 PDT
(In reply to Ryan Haddad from comment #9)
> (In reply to Mark Lam from comment #8)
> > Thanks for the review.  Landed in r216428: <http://trac.webkit.org/r216428>.
> 
> It appears that this change broke the Windows Debug build:
> https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/
> 1120

Speculative build fix landed in r216444: <http://trac.webkit.org/r216444>.