Bug 156632

Summary: Reduce use of Deprecated::ScriptXXX classes
Product: WebKit Reporter: Darin Adler <darin>
Component: WebCore JavaScriptAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, buildbot, cdumez, ggaren, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch achristensen: review+

Description Darin Adler 2016-04-15 11:40:58 PDT
Reduce use of Deprecated::ScriptXXX classes
Comment 1 Darin Adler 2016-04-15 12:35:45 PDT
Created attachment 276494 [details]
Patch
Comment 2 Darin Adler 2016-04-15 13:10:04 PDT
Looks like I missed a null check somewhere.
Comment 3 Build Bot 2016-04-15 13:13:02 PDT
Comment on attachment 276494 [details]
Patch

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

New failing tests:
fast/dom/javascript-url-exception-isolation.html
fast/frames/sandboxed-iframe-scripting-03.html
Comment 4 Build Bot 2016-04-15 13:13:04 PDT
Created attachment 276499 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 5 Build Bot 2016-04-15 13:13:14 PDT
Comment on attachment 276494 [details]
Patch

Attachment 276494 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1164765

New failing tests:
fast/dom/javascript-url-exception-isolation.html
fast/frames/sandboxed-iframe-scripting-03.html
Comment 6 Build Bot 2016-04-15 13:13:17 PDT
Created attachment 276500 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-04-15 13:28:14 PDT
Comment on attachment 276494 [details]
Patch

Attachment 276494 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1164777

New failing tests:
fast/dom/javascript-url-exception-isolation.html
fast/frames/sandboxed-iframe-scripting-03.html
Comment 8 Build Bot 2016-04-15 13:28:17 PDT
Created attachment 276502 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Build Bot 2016-04-15 13:33:27 PDT
Comment on attachment 276494 [details]
Patch

Attachment 276494 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1164830

New failing tests:
fast/dom/javascript-url-exception-isolation.html
fast/frames/sandboxed-iframe-scripting-03.html
Comment 10 Build Bot 2016-04-15 13:33:30 PDT
Created attachment 276503 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 11 Darin Adler 2016-04-15 14:36:22 PDT
Created attachment 276514 [details]
Patch
Comment 12 Alex Christensen 2016-04-15 16:32:05 PDT
Comment on attachment 276514 [details]
Patch

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

> Source/JavaScriptCore/bindings/ScriptValue.cpp:169
>  RefPtr<InspectorValue> ScriptValue::toInspectorValue(ExecState* scriptState) const
>  {
>      JSLockHolder holder(scriptState);
> -    return jsToInspectorValue(scriptState, m_value.get(), InspectorValue::maxDepth);
> +    return jsToInspectorValue(*scriptState, m_value.get(), InspectorValue::maxDepth);
>  }

There are a lot of places where ExecStates and VMs are passed around as pointers that are assumed to be non-null.  Why don't we use more ExecState&?

> Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:87
> -    ScriptGlobalObject::set(execStateFromPage(mainThreadNormalWorld(), m_page.corePage()), ASCIILiteral("InspectorFrontendHost"), m_frontendHost.get());
> +    ScriptGlobalObject::set(*execStateFromPage(mainThreadNormalWorld(), m_page.corePage()), ASCIILiteral("InspectorFrontendHost"), *m_frontendHost);

This will be dereferencing a null pointer if corePage() returns null.
Comment 13 Darin Adler 2016-04-15 19:25:45 PDT
Committed r199619: <http://trac.webkit.org/changeset/199619>
Comment 14 Darin Adler 2016-04-15 19:27:28 PDT
Comment on attachment 276514 [details]
Patch

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

>> Source/JavaScriptCore/bindings/ScriptValue.cpp:169
>>  }
> 
> There are a lot of places where ExecStates and VMs are passed around as pointers that are assumed to be non-null.  Why don't we use more ExecState&?

Yes, there are. We should use more ExecState&. It takes a long time to change all the old code.

>> Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:87
>> +    ScriptGlobalObject::set(*execStateFromPage(mainThreadNormalWorld(), m_page.corePage()), ASCIILiteral("InspectorFrontendHost"), *m_frontendHost);
> 
> This will be dereferencing a null pointer if corePage() returns null.

Yes, was true before, the dereferencing was inside ScriptGlobalObject::set, and it is equally true now.