Bug 154350 - SES selftest page crashes on nightly r196694
Summary: SES selftest page crashes on nightly r196694
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
: 154349 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-02-17 12:19 PST by Mark S. Miller
Modified: 2016-02-21 20:04 PST (History)
10 users (show)

See Also:


Attachments
Screenshots of Nightly before and after bug (782.32 KB, image/png)
2016-02-17 12:19 PST, Mark S. Miller
no flags Details
Patch (5.45 KB, patch)
2016-02-17 14:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
SES selftest page now: A problem occurred with this webpage so it was reloaded (171.28 KB, image/png)
2016-02-17 19:35 PST, Mark S. Miller
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark S. Miller 2016-02-17 12:19:05 PST
Created attachment 271577 [details]
Screenshots of Nightly before and after bug

Visit https://rawgit.com/tvcutsem/es-lab/master/src/ses/contract.html
in the latest Webkit.

As shown on the attachment, it worked fine on Nightly r196074, so the
problem happened since then.
Comment 1 Keith Miller 2016-02-17 12:44:12 PST
This looks like it's probably because of http://trac.webkit.org/changeset/196220. That's the commit that has modified the crashing function, getOwnPropertyDescriptor, this year.
Comment 2 Radar WebKit Bug Importer 2016-02-17 12:46:10 PST
<rdar://problem/24704334>
Comment 3 Radar WebKit Bug Importer 2016-02-17 12:46:14 PST
<rdar://problem/24704338>
Comment 4 Chris Dumez 2016-02-17 13:02:39 PST
Likely to be a regression from:
http://trac.webkit.org/changeset/196001
or
http://trac.webkit.org/changeset/196145
Comment 5 Chris Dumez 2016-02-17 13:30:57 PST
I am working on this.
Comment 6 Chris Dumez 2016-02-17 14:22:00 PST
Created attachment 271592 [details]
Patch
Comment 7 Mark Lam 2016-02-17 14:57:08 PST
Comment on attachment 271592 [details]
Patch

r=me
Comment 8 Chris Dumez 2016-02-17 14:59:56 PST
Comment on attachment 271592 [details]
Patch

Clearing flags on attachment: 271592

Committed r196723: <http://trac.webkit.org/changeset/196723>
Comment 9 Chris Dumez 2016-02-17 15:00:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Mark S. Miller 2016-02-17 19:33:36 PST
As of r196733 I am now seeing "A problem occurred with this webpage so it was reloaded." most times when I visit https://rawgit.com/tvcutsem/es-lab/master/src/ses/contract.html . It doesn't happen every time. But if I bring up the web inspector, set a breakpoint, and then reload, then it does happen every time closing the web inspector in the process, preventing me from catching ses at a breakpoint.

Will attach a screenshot momentarily.

Should I reopen this bug or file a fresh one?
Comment 11 Mark S. Miller 2016-02-17 19:35:10 PST
Created attachment 271617 [details]
SES selftest page now: A problem occurred with this webpage so it was reloaded
Comment 12 Chris Dumez 2016-02-17 19:37:24 PST
(In reply to comment #11)
> Created attachment 271617 [details]
> SES selftest page now: A problem occurred with this webpage so it was
> reloaded

Ok, I will take another look and see if I can reproduce. Thank you for verifying the fix.
Comment 13 Chris Dumez 2016-02-17 19:40:34 PST
(In reply to comment #11)
> Created attachment 271617 [details]
> SES selftest page now: A problem occurred with this webpage so it was
> reloaded

Also, a backtrace is more useful than a screenshot :) You can get the backtrace the .crash file for the com.apple.WebKit.WebContent process from the "console" utility.
Comment 14 Chris Dumez 2016-02-17 19:52:48 PST
Looks like a can reproduce the crash but I have to open Web Inspector and reload the page. I filed https://bugs.webkit.org/show_bug.cgi?id=154378 to track it.
Comment 15 Alexey Proskuryakov 2016-02-17 21:30:45 PST
*** Bug 154349 has been marked as a duplicate of this bug. ***
Comment 16 Darin Adler 2016-02-21 20:04:32 PST
Comment on attachment 271592 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.h:1231
> -            if ((attributes & Accessor) != (currentAttributes & Accessor)) {
> +            if ((attributes & Accessor) != (currentAttributes & Accessor) || (attributes & CustomAccessor) != (currentAttributes & CustomAccessor)) {

Here’s how I’d write it:

    auto accessAttributes = Accessor | CustomAccessor;
    if ((attributes & accessAttributes) != (currentAttributes & accessAttributes)) {

Better than the || I think.