Bug 164996 - Fix exception scope verification failures in *Executable.cpp files.
Summary: Fix exception scope verification failures in *Executable.cpp files.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-11-19 18:20 PST by Mark Lam
Modified: 2016-11-21 09:20 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (7.29 KB, patch)
2016-11-19 18:32 PST, Mark Lam
darin: 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 2016-11-19 18:20:11 PST
Patch coming.
Comment 1 Mark Lam 2016-11-19 18:32:53 PST
Created attachment 295275 [details]
proposed patch.
Comment 2 Darin Adler 2016-11-20 16:08:48 PST
Comment on attachment 295275 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ProgramExecutable.cpp:82
>      JSObject* exception = 0;

nullptr

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:314
> +        ExecState* exec = scope->globalObject()->globalExec();

I would write:

    auto& state = *scope->globalObject()->globalExec();

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:318
>      JSObject* exception = 0;

nullptr
Comment 3 Saam Barati 2016-11-20 19:17:36 PST
Comment on attachment 295275 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:314
>> +        ExecState* exec = scope->globalObject()->globalExec();
> 
> I would write:
> 
>     auto& state = *scope->globalObject()->globalExec();

I think making this change would go against the style of what we usually do inside JSC. I'm not against using this style throughout JSC, however, I think it's more important that we be consistent. I think at some point it's worth all of us having a more detailed discussion about coding style in JSC. I know that we mostly follow the normal WebKit style, however, I've noticed that we've differed from that style in some ways. Notably, I think JSC uses "auto" much less than the rest of WebKit does.

In this specific line of code, I usually see it written as Mark has written it. We tend to call things of type "ExecState*" exec, and not declare it using auto.
Comment 4 Mark Lam 2016-11-21 09:11:22 PST
Comment on attachment 295275 [details]
proposed patch.

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

>>> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:314
>>> +        ExecState* exec = scope->globalObject()->globalExec();
>> 
>> I would write:
>> 
>>     auto& state = *scope->globalObject()->globalExec();
> 
> I think making this change would go against the style of what we usually do inside JSC. I'm not against using this style throughout JSC, however, I think it's more important that we be consistent. I think at some point it's worth all of us having a more detailed discussion about coding style in JSC. I know that we mostly follow the normal WebKit style, however, I've noticed that we've differed from that style in some ways. Notably, I think JSC uses "auto" much less than the rest of WebKit does.
> 
> In this specific line of code, I usually see it written as Mark has written it. We tend to call things of type "ExecState*" exec, and not declare it using auto.

I think I'll go ahead and cave, and switch to using the convention of using ExecState& state instead where possible since I'll be touching a lot of code in my exception checks series of patches.  I don't see the WebKit project switching back to using "exec".  So, might as well get started on the switch.
Comment 5 Mark Lam 2016-11-21 09:20:59 PST
Thanks for the reviews.  Landed in r208950: <http://trac.webkit.org/r208950>.