Bug 156539 - Remove UsePointersEvenForNonNullableObjectArguments from Internals
Summary: Remove UsePointersEvenForNonNullableObjectArguments from Internals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-13 08:52 PDT by Darin Adler
Modified: 2016-04-14 10:10 PDT (History)
7 users (show)

See Also:


Attachments
Patch (102.83 KB, patch)
2016-04-13 09:08 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (103.28 KB, patch)
2016-04-13 09:25 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (100.79 KB, patch)
2016-04-13 21:43 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2016-04-13 08:52:16 PDT
Remove UsePointersEvenForNonNullableObjectArguments from Internals
Comment 1 Darin Adler 2016-04-13 09:08:43 PDT
Created attachment 276325 [details]
Patch
Comment 2 Chris Dumez 2016-04-13 09:22:04 PDT
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
Comment 3 Darin Adler 2016-04-13 09:25:06 PDT
Created attachment 276327 [details]
Patch
Comment 4 Darin Adler 2016-04-13 19:46:52 PDT
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?
Comment 5 Alex Christensen 2016-04-13 21:43:45 PDT
Created attachment 276372 [details]
Patch
Comment 6 Brent Fulgham 2016-04-13 21:46:54 PDT
(In reply to comment #5)
> Created attachment 276372 [details]
> Patch

What did you change? I'm still in the process of building! :-)
Comment 7 Alex Christensen 2016-04-13 21:57:36 PDT
(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.
Comment 8 Alex Christensen 2016-04-13 23:19:09 PDT
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?
Comment 9 Darin Adler 2016-04-14 09:18:35 PDT
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.
Comment 10 Darin Adler 2016-04-14 09:19:58 PDT
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.
Comment 11 WebKit Commit Bot 2016-04-14 10:10:05 PDT
Comment on attachment 276372 [details]
Patch

Clearing flags on attachment: 276372

Committed r199539: <http://trac.webkit.org/changeset/199539>
Comment 12 WebKit Commit Bot 2016-04-14 10:10:09 PDT
All reviewed patches have been landed.  Closing bug.