Bug 156539 – Remove UsePointersEvenForNonNullableObjectArguments from Internals

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Darin Adler]

Remove UsePointersEvenForNonNullableObjectArguments from Internals

[Darin Adler]

Created attachment 276325 [details]
Patch

[Chris Dumez]

Does not build on GTK and EFL:
../../Source/WebCore/testing/Internals.cpp
../../Source/WebCore/testing/Internals.cpp:3260:8: error: prototype for 'WTF::String WebCore::Internals::userVisibleString(const WebCore::DOMURL*)' does not match any in class 'WebCore::Internals'
In file included from ../../Source/WebCore/testing/Internals.cpp:28:0:
../../Source/WebCore/testing/Internals.h:468:12: error: candidate is: WTF::String WebCore::Internals::userVisibleString(const WebCore::DOMURL&)
ICECC[11745] 15:46:28: Compiled on 192.168.10.38
Copying template files to output directory...
Running gtkdoc-scan
Running gtkdoc-scangobj
Running gtkdoc-mkdb

[Darin Adler]

Created attachment 276327 [details]
Patch

[Darin Adler]

Alex, Brent, can one of you help me fix the Windows build failure? It seems that somehow WEBCORE_TESTSUPPORT_EXPORT is not defined. When I look in PlatformExportMacros.h, it says "// Windows must set this per-project". What does that mean?

[Alex Christensen]

Created attachment 276372 [details]
Patch

[Brent Fulgham]

(In reply to comment #5)
> Created attachment 276372 [details]
> Patch

What did you change? I'm still in the process of building! :-)

[Alex Christensen]

(In reply to comment #4)
> Alex, Brent, can one of you help me fix the Windows build failure? It seems
> that somehow WEBCORE_TESTSUPPORT_EXPORT is not defined. When I look in
> PlatformExportMacros.h, it says "// Windows must set this per-project". What
> does that mean?
I don't remember all the details, but WEBCORE_EXPORT is defined in WebCorePrefix.h and WebCoreTestSupportPrefix.h on Windows because it needs to be different. WEBCORE_TESTSUPPORT_EXPORT is only defined in WebCoreTestSupportPrefix.h, and it is only used in WebCoreTestSupport.  I think the correct solution here is to use WEBCORE_EXPORT here for the HTML Elements that are in WebCore, and WEBCORE_TESTSUPPORT_EXPORT for Internals.idl, which is in WebCoreTestSupport.  I uploaded a patch that does that.

There was an idea going around to make a new export macro for functions in WebCore that exist only for testing.  It would export the symbols for engineering builds and not for production builds so that code elimination would be more effective and reduce the binary size of production builds, but we have not done that yet.  And we were uncertain if it would be worth the build breakages caused by using the wrong macro.

[Alex Christensen]

Comment on attachment 276372 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:477
> -    return String::format("%p", node);
> +    return String::format("%p", &node);

The existence of this function could have some scary security implications if this is accessible in released software (which it's not).  Maybe we should remove it, though.

> Source/WebCore/testing/Internals.cpp:1237
> +    FrameView* frameView = element.document().view();

auto

> Source/WebCore/testing/Internals.cpp:1247
> +    if (!is<HTMLFormControlElement>(element)) {

It seems like we could make this parameter an HTMLFormControlElement, similar to how we are doing with other functions like setShowAutoFillButton.  Then we wouldn't need this.

> LayoutTests/fast/forms/color/input-color-onchange-event.html:-40
> -evalAndLog("internals.selectColorInColorChooser({}, '#ff0000');");
> -evalAndLog("internals.selectColorInColorChooser(document, '#ff0000');");

Do these throw something now?  Would putting the function in a try statement still successfully test the behavior of invalid parameters?

[Darin Adler]

Comment on attachment 276372 [details]
Patch

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

>> Source/WebCore/testing/Internals.cpp:477
>> +    return String::format("%p", &node);
> 
> The existence of this function could have some scary security implications if this is accessible in released software (which it's not).  Maybe we should remove it, though.

First of all, I think this is not a real concern. There are all sorts of security problems with internals, and none of it is safe to ship. This is just one of the most obvious bad functions, but others are terrible for security in many different ways.

But if we do want to wipe out this particular function because of an excess of caution, the one place that uses it is resources/dump-as-markup.js so you could discuss other ways to make that work with Ryosuke.

An easy thing to do that might mitigate the risk a little would be to compute a SHA-1 digest of the pointer along with a randomly generated integer; just needs to be a unique identifier, doesn’t need to actually be the pointer.

>> Source/WebCore/testing/Internals.cpp:1247
>> +    if (!is<HTMLFormControlElement>(element)) {
> 
> It seems like we could make this parameter an HTMLFormControlElement, similar to how we are doing with other functions like setShowAutoFillButton.  Then we wouldn't need this.

Yes, I considered that. We could do that HTMLFormControlElement was a DOM type, but it’s not. It’s an internal type and not web-exposed.

>> LayoutTests/fast/forms/color/input-color-onchange-event.html:-40
>> -evalAndLog("internals.selectColorInColorChooser(document, '#ff0000');");
> 
> Do these throw something now?  Would putting the function in a try statement still successfully test the behavior of invalid parameters?

Yes, I considered that, but this is not a useful test. There is no real value in testing edge cases of the internals functions themselves.

[Darin Adler]

Comment on attachment 276372 [details]
Patch

I’m going to commit-queue this; doesn’t seem critical to make the one change I want to do (use auto for FrameView*) and I don’t currently have a convenient branch to do this on.

[WebKit Commit Bot]

Comment on attachment 276372 [details]
Patch

Clearing flags on attachment: 276372

Committed r199539: <http://trac.webkit.org/changeset/199539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.