Bug 181351 – Add a DOM gadget for Spectre testing

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Michael Saboff]

A "gadget" added to a DOM node type would allow us to test DOM binding and supporting code mitigations.

[Radar WebKit Bug Importer]

<rdar://problem/36329006>

[Michael Saboff]

Created attachment 330601 [details]
Patch

[EWS Watchlist]

Attachment 330601 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Comment.cpp:89:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: vulnerab  [changelog/unwantedsecurityterms] [3]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 330601 [details]
Patch

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

> Source/WebCore/dom/Comment.h:45
> +    const unsigned m_maxDataLength = 100u;

lets make this constexpr

[EWS Watchlist]

Comment on attachment 330601 [details]
Patch

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

New failing tests:
inspector/model/remote-object-dom.html

[EWS Watchlist]

Created attachment 330608 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330601 [details]
Patch

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

New failing tests:
inspector/model/remote-object-dom.html

[EWS Watchlist]

Created attachment 330616 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330601 [details]
Patch

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

New failing tests:
svg/wicd/rightsizing-grid.xhtml
inspector/model/remote-object-dom.html

[EWS Watchlist]

Created attachment 330624 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Michael Saboff]

Created attachment 330742 [details]
Patch for landing - took care of review feedback and fixed test failure

[EWS Watchlist]

Attachment 330742 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Comment.cpp:91:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 330742 [details]
Patch for landing - took care of review feedback and fixed test failure

Clearing flags on attachment: 330742

Committed r226600: <https://trac.webkit.org/changeset/226600>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

This is breaking windows builds and looks like iOS as well.

[Alexey Proskuryakov]

Comment on attachment 330742 [details]
Patch for landing - took care of review feedback and fixed test failure

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

> Source/WebCore/dom/Comment.h:47
> +    Vector<int32_t> m_data;
> +    size_t m_readLength;
> +    int32_t* m_dataPtr;

It still feels weird to me to pay a runtime cost for the test in customer builds.

[Ryosuke Niwa]

Comment on attachment 330742 [details]
Patch for landing - took care of review feedback and fixed test failure

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

> Source/WebCore/dom/Comment.h:45
> +    Vector<int32_t> m_data;

Wow, we can't do this to a comment node.
Websites have a lot of comment nodes.

[Saam Barati]

(In reply to Alexey Proskuryakov from comment #16)
> Comment on attachment 330742 [details]
> Patch for landing - took care of review feedback and fixed test failure
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330742&action=review
> 
> > Source/WebCore/dom/Comment.h:47
> > +    Vector<int32_t> m_data;
> > +    size_t m_readLength;
> > +    int32_t* m_dataPtr;
> 
> It still feels weird to me to pay a runtime cost for the test in customer
> builds.

Michael has a plan to only do this for engineering builds.

[Saam Barati]

(In reply to Ryosuke Niwa from comment #17)
> Comment on attachment 330742 [details]
> Patch for landing - took care of review feedback and fixed test failure
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330742&action=review
> 
> > Source/WebCore/dom/Comment.h:45
> > +    Vector<int32_t> m_data;
> 
> Wow, we can't do this to a comment node.
> Websites have a lot of comment nodes.

Michael has a plan to only do this for engineering builds.

[Saam Barati]

(In reply to Saam Barati from comment #19)
> (In reply to Ryosuke Niwa from comment #17)
> > Comment on attachment 330742 [details]
> > Patch for landing - took care of review feedback and fixed test failure
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=330742&action=review
> > 
> > > Source/WebCore/dom/Comment.h:45
> > > +    Vector<int32_t> m_data;
> > 
> > Wow, we can't do this to a comment node.
> > Websites have a lot of comment nodes.
> 
> Michael has a plan to only do this for engineering builds.

That said, maybe this will hurt our ability to test. If so, we should probably just add brand new internal API for spectre testing. Shouldn't be hard.

[Alexey Proskuryakov]

Can we roll back until the better solution can be designed?

[Michael Saboff]

(In reply to Alexey Proskuryakov from comment #21)
> Can we roll back until the better solution can be designed?

Rolled out in r226658.

[Michael Saboff]

Created attachment 330969 [details]
Patch

[EWS Watchlist]

Attachment 330969 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SpectreGadget.cpp:72:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 330976 [details]
Patch with speculative build fix for GTK

[EWS Watchlist]

Attachment 330976 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SpectreGadget.cpp:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Please rename to InspectreGadget.

[EWS Watchlist]

Comment on attachment 330976 [details]
Patch with speculative build fix for GTK

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

New failing tests:
accessibility/mac/aria-multiple-liveregions-notification.html

[EWS Watchlist]

Created attachment 331010 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 330976 [details]
Patch with speculative build fix for GTK

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[EWS Watchlist]

Created attachment 331012 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Joseph Pecoraro]

Comment on attachment 330976 [details]
Patch with speculative build fix for GTK

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

Much better!

> Source/WebCore/Sources.txt:787
> +dom/SpectreGadget.cpp

Style: Not in alphabetical order like all the rest.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1795
> +		657AFAFC20047A2900509464 /* SpectreGadget.h in Headers */ = {isa = PBXBuildFile; fileRef = 657AFAF82004789900509464 /* SpectreGadget.h */; settings = {ATTRIBUTES = (Private, ); }; };

This doesn't appear to need to be Private. Nobody includes this outside of WebCore, right?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:26238
> +				657AFAFC20047A2900509464 /* SpectreGadget.h in Headers */,

This section could be sorted:

    ./Tools/Scripts/sort-Xcode-project-file Source/WebCore/WebCore.xcodeproj/project.pbxproj

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:142
> +    macro(SpectreGadget) \

What is this one needed for? Is there builtin code for SpectreGadget? I would only expect this would be needed if there was an @SpectreGadget use in builtins or a need for SpectreGadgetPrivateName.

> Source/WebCore/dom/SpectreGadget.h:26
> +
> +

Style: Too many empty lines

> Source/WebCore/dom/SpectreGadget.idl:28
> +    // Conditional=PREPROCESS_MACRO

No need for this comment, please remove.

> Source/WebCore/dom/SpectreGadget.idl:31
> +    JSGenerateToJSObject,

This doesn't appear to be necessary, I don't see any code that converts this class to a JSObject explicitly.

[Michael Saboff]

(In reply to Joseph Pecoraro from comment #32)
> Comment on attachment 330976 [details]
> Patch with speculative build fix for GTK
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330976&action=review
> 
> Much better!
> 
> > Source/WebCore/Sources.txt:787
> > +dom/SpectreGadget.cpp
> 
> Style: Not in alphabetical order like all the rest.

Fixed.
 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:1795
> > +		657AFAFC20047A2900509464 /* SpectreGadget.h in Headers */ = {isa = PBXBuildFile; fileRef = 657AFAF82004789900509464 /* SpectreGadget.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> This doesn't appear to need to be Private. Nobody includes this outside of
> WebCore, right?

Made it Project.

> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26238
> > +				657AFAFC20047A2900509464 /* SpectreGadget.h in Headers */,
>
> This section could be sorted:

Done.
 
>     ./Tools/Scripts/sort-Xcode-project-file
> Source/WebCore/WebCore.xcodeproj/project.pbxproj
> 
> > Source/WebCore/bindings/js/WebCoreBuiltinNames.h:142
> > +    macro(SpectreGadget) \
> 
> What is this one needed for? Is there builtin code for SpectreGadget? I
> would only expect this would be needed if there was an @SpectreGadget use in
> builtins or a need for SpectreGadgetPrivateName.

It is needed to add the constructor to the GlobalObject.

> > Source/WebCore/dom/SpectreGadget.h:26
> > +
> > +
> 
> Style: Too many empty lines

Fixed

> > Source/WebCore/dom/SpectreGadget.idl:28
> > +    // Conditional=PREPROCESS_MACRO
> 
> No need for this comment, please remove.

For the future, but I removed it.

> > Source/WebCore/dom/SpectreGadget.idl:31
> > +    JSGenerateToJSObject,
> 
> This doesn't appear to be necessary, I don't see any code that converts this
> class to a JSObject explicitly.

Copy / paste error - removed.

[Joseph Pecoraro]

Comment on attachment 330976 [details]
Patch with speculative build fix for GTK

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

>>> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:142
>>> +    macro(SpectreGadget) \
>> 
>> What is this one needed for? Is there builtin code for SpectreGadget? I would only expect this would be needed if there was an @SpectreGadget use in builtins or a need for SpectreGadgetPrivateName.
> 
> It is needed to add the constructor to the GlobalObject.

Hmm, that doesn't make sense to me. That is the [Exposed=Window] IDL interface attribute should do.

This file appears to be specific to "Builtins". What fails when you remove this line?

[Michael Saboff]

(In reply to Joseph Pecoraro from comment #34)
> Comment on attachment 330976 [details]
> Patch with speculative build fix for GTK
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330976&action=review
> 
> >>> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:142
> >>> +    macro(SpectreGadget) \
> >> 
> >> What is this one needed for? Is there builtin code for SpectreGadget? I would only expect this would be needed if there was an @SpectreGadget use in builtins or a need for SpectreGadgetPrivateName.
> > 
> > It is needed to add the constructor to the GlobalObject.
> 
> Hmm, that doesn't make sense to me. That is the [Exposed=Window] IDL
> interface attribute should do.
> 
> This file appears to be specific to "Builtins". What fails when you remove
> this line?

In file included from /Volumes/Data/src/webkit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource19.cpp:5:
/Volumes/Data/src/webkit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMWindow.cpp:6299:97: error: no member named 'SpectreGadgetPublicName' in 'WebCore::WebCoreBuiltinNames'
        putDirectCustomAccessor(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().SpectreGadgetPublicName(), CustomGetterSetter::create(vm, jsDOMWindowSpectreGadgetConstructor, setJSDOMWindowSpectreGadgetConstructor), attributesForStructure(static_cast<unsigned>(JSC::PropertyAttribute::DontEnum)));
                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
1 error generated.

[Joseph Pecoraro]

Comment on attachment 330976 [details]
Patch with speculative build fix for GTK

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

>>>>> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:142
>>>>> +    macro(SpectreGadget) \
>>>> 
>>>> What is this one needed for? Is there builtin code for SpectreGadget? I would only expect this would be needed if there was an @SpectreGadget use in builtins or a need for SpectreGadgetPrivateName.
>>> 
>>> It is needed to add the constructor to the GlobalObject.
>> 
>> Hmm, that doesn't make sense to me. That is the [Exposed=Window] IDL interface attribute should do.
>> 
>> This file appears to be specific to "Builtins". What fails when you remove this line?
> 
> In file included from /Volumes/Data/src/webkit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource19.cpp:5:
> /Volumes/Data/src/webkit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMWindow.cpp:6299:97: error: no member named 'SpectreGadgetPublicName' in 'WebCore::WebCoreBuiltinNames'
>         putDirectCustomAccessor(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().SpectreGadgetPublicName(), CustomGetterSetter::create(vm, jsDOMWindowSpectreGadgetConstructor, setJSDOMWindowSpectreGadgetConstructor), attributesForStructure(static_cast<unsigned>(JSC::PropertyAttribute::DontEnum)));
>                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> 1 error generated.

Oh very interesting! So apparently the *PublicName this is needed when the interface has the EnabledAtRuntime IDL attribute. That is good for me to know in the future. Thanks!

[Joseph Pecoraro]

I was mislead by WebCoreBuiltinNames's WEBCORE_COMMON_PRIVATE_IDENTIFIERS_EACH_PROPERTY_NAME macro. Despite saying _PRIVATE_ it is used to create both a PublicName and PrivateName identifier. In some cases this wastes time and bytes creating identifiers that won't ever be needed or used. But this is no longer relevant to your patch.

[Michael Saboff]

Committed r226778: <https://trac.webkit.org/changeset/226778>