Bug 223309 – modern-media-controls script should not be allocated in heap

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

We should not allocate heap memory.
And we should not concatenate localized string data.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #0)
> We should not allocate heap memory.
> And we should not concatenate localized string data.

Currently, I have no idea how to change localized string part. We should not concatenate them. We should

1. Have JSON data for localized strings
2. Pass it to media control script
3. And parse it inside media control script

This localized string part makes script UTF-16, this is bad for memory.
In this bug, let's just first minify the script.

[Yusuke Suzuki]

Created attachment 423460 [details]
Patch

[Yusuke Suzuki]

Created attachment 423463 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75520193>

[Eric Carlson]

Comment on attachment 423463 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";

I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.

[Alexey Proskuryakov]

Comment on attachment 423463 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36071
> +				"$(SRCROOT)/../JavaScriptCore/Scripts/js-minify.py",

Is this really the only input path? And why are there no output paths?

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
> 
> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.

A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?

[Yusuke Suzuki]

Comment on attachment 423463 [details]
Patch

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36071
>> +				"$(SRCROOT)/../JavaScriptCore/Scripts/js-minify.py",
> 
> Is this really the only input path? And why are there no output paths?

Maybe we should add these files to output files? They are not included before this patch.

>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>> 
>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
> 
> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?

I think we should check Debug build or not. This is what 

#if !defined(NDEBUG)
        // Setting a scriptURL allows the source to be debuggable in the inspector.
        URL scriptURL = URL({ }, "mediaControlsScript"_s);
            // Setting a scriptURL allows the source to be debuggable in the inspector.
            URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
#else
        URL scriptURL;
            URL scriptURL;
#endif

part is doing.

[Devin Rousso]

Comment on attachment 423463 [details]
Patch

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

>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>> 
>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>> 
>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
> 
> I think we should check Debug build or not. This is what 
> 
> #if !defined(NDEBUG)
>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
> #else
>         URL scriptURL;
>             URL scriptURL;
> #endif
> 
> part is doing.

I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.

Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.

> Source/WebCore/rendering/RenderThemeIOS.mm:1296
> +            m_mediaControlsAdditionalScript = String(RenderThemeIOSAdditions_mediaControlsScript);

you'll need to change `RenderThemeIOSAdditions_mediaControlsScript` in WebKitAdditions

> Source/WebCore/rendering/RenderThemeMac.mm:338
> +            m_mediaControlsAdditionalScript = String(RenderThemeMacAdditions_mediaControlsScript);

you'll need to change `RenderThemeMacAdditions_mediaControlsScript` in WebKitAdditions

[Devin Rousso]

Comment on attachment 423463 [details]
Patch

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

>>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>>> 
>>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>>> 
>>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
>> 
>> I think we should check Debug build or not. This is what 
>> 
>> #if !defined(NDEBUG)
>>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
>> #else
>>         URL scriptURL;
>>             URL scriptURL;
>> #endif
>> 
>> part is doing.
> 
> I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.
> 
> Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.

As an alternative to making the visibility of the media controls script based on a compile flag, I think you could name it something like `__InjectedScript_mediaControls.js` which would be ignored by Web Inspector unless the "Show WebKit-internal scripts" engineering setting (which are only shown for local engineering builds) is enabled.

[Yusuke Suzuki]

Comment on attachment 423463 [details]
Patch

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

>>>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>>>> 
>>>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>>>> 
>>>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
>>> 
>>> I think we should check Debug build or not. This is what 
>>> 
>>> #if !defined(NDEBUG)
>>>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>>>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
>>> #else
>>>         URL scriptURL;
>>>             URL scriptURL;
>>> #endif
>>> 
>>> part is doing.
>> 
>> I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.
>> 
>> Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.
> 
> As an alternative to making the visibility of the media controls script based on a compile flag, I think you could name it something like `__InjectedScript_mediaControls.js` which would be ignored by Web Inspector unless the "Show WebKit-internal scripts" engineering setting (which are only shown for local engineering builds) is enabled.

Do we have a Xcode environment variable which can be switched based on "development build"?

[Yusuke Suzuki]

Comment on attachment 423463 [details]
Patch

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

>>>>>>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:36078
>>>>>>> +			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' | python \"$SRCROOT/../JavaScriptCore/Scripts/js-minify.py\" > \"$DST_DIR/modern-media-controls.js\"\n";
>>>>>> 
>>>>>> I can't tell if this is already the case, but it would be helpful to not minify the sources in development builds.
>>>>> 
>>>>> A big problem with this is that there isn't a great definition of what a "development build" is. Just debug vs. release, or something else?
>>>> 
>>>> I think we should check Debug build or not. This is what 
>>>> 
>>>> #if !defined(NDEBUG)
>>>>         // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>>         URL scriptURL = URL({ }, "mediaControlsScript"_s);
>>>>             // Setting a scriptURL allows the source to be debuggable in the inspector.
>>>>             URL scriptURL = URL({ }, makeString("mediaControlsScript"_s, index));
>>>> #else
>>>>         URL scriptURL;
>>>>             URL scriptURL;
>>>> #endif
>>>> 
>>>> part is doing.
>>> 
>>> I think @Eric Carlson meant "development build" more as "engineering build" which is any time an engineer does a local build.
>>> 
>>> Having it be debug-only is really not ideal since 99% of features involving media controls do not need assertions or anything like that.
>> 
>> As an alternative to making the visibility of the media controls script based on a compile flag, I think you could name it something like `__InjectedScript_mediaControls.js` which would be ignored by Web Inspector unless the "Show WebKit-internal scripts" engineering setting (which are only shown for local engineering builds) is enabled.
> 
> Do we have a Xcode environment variable which can be switched based on "development build"?

Or, I'll just drop minification thing from this patch since this does not offer memory reduction (this reduces size of framework, but this is not a main goal of this patch).

[Yusuke Suzuki]

Created attachment 423512 [details]
Patch

[Yusuke Suzuki]

Created attachment 423513 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 423513 [details]
Patch

Ah, let me wait. It seems that we are allocating memory for String(CFString). We should serialize script into C characters as we do in builtin code and place it into __DATA_CONST.

[Alexey Proskuryakov]

> Maybe we should add these files to output files? They are not included before this patch.

1. Input files. Seems certain that we need more here, as otherwise incremental builds will not be triggered when inputs change.

2. Output files. This script seems to put outputs directly in their final location, so nothing else probably depends on it. I don't know what old and new Xcode build systems will do when output is missing. Will they run this script phase unconditionally, wasting incremental build time even when nothing changes?

As for engineering builds, strategically there should be as little difference between local builds and what ships to customers as possible. Things like minification in particular tend to have bugs, and we've seen a few before. I acknowledge the need to debug, yet I would encourage thinking of ways to make these scripts debuggable in some other way, that's not automagically triggered by "local builds".

[Devin Rousso]

I think my suggestion of using `__InjectedScript_*.js` might actually be a valid alternative that satisfies both cases because that will not only be hidden from Web Inspector in non-local builds but will also be ignored by the debugger when pausing (local engineering builds can enable the "Pause in WebKit-internal scripts" setting in Web Inspector).

[Yusuke Suzuki]

(In reply to Alexey Proskuryakov from comment #15)
> > Maybe we should add these files to output files? They are not included before this patch.
> 
> 1. Input files. Seems certain that we need more here, as otherwise
> incremental builds will not be triggered when inputs change.
> 
> 2. Output files. This script seems to put outputs directly in their final
> location, so nothing else probably depends on it. I don't know what old and
> new Xcode build systems will do when output is missing. Will they run this
> script phase unconditionally, wasting incremental build time even when
> nothing changes?
> 
> As for engineering builds, strategically there should be as little
> difference between local builds and what ships to customers as possible.
> Things like minification in particular tend to have bugs, and we've seen a
> few before. I acknowledge the need to debug, yet I would encourage thinking
> of ways to make these scripts debuggable in some other way, that's not
> automagically triggered by "local builds".

Since WTFString(CFString) will allocate memory anyway, I'll go to the different course, basically just use UserAgentScripts as we already do for QuickTimePluginReplacement.js since UserAgentScripts is designed for this `user-agent specific script source` purpose.

[Yusuke Suzuki]

Created attachment 423520 [details]
Patch

[Yusuke Suzuki]

Created attachment 423522 [details]
Patch

[Devin Rousso]

Comment on attachment 423522 [details]
Patch

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

r=me once EWS is happy, nice fix! :)

> Source/WebCore/Modules/modern-media-controls/js-files:-1
> -main.js

This file is also used by `LayoutTests/media/modern-media-controls/resources/media-controls-loader.js`.  IMO it's fine to copy this list into that file though (which would also avoid a sync XHR).

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-36084
> -			shellScript = "SRC_DIR=\"$SRCROOT/Modules/modern-media-controls\"\nDST_DIR=\"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH/modern-media-controls\"\n\nmkdir -p \"$DST_DIR\"\n\nif [ \"${WK_PLATFORM_NAME}\" == \"macosx\" ]; then\n    IMG_OS_PREFIX=\"macOS\"\nelse\n    if [[ \"${WK_PLATFORM_NAME}\" == *\"watch\"* ]]; then\n        IMG_OS_PREFIX=\"watchOS\"\n    else\n        IMG_OS_PREFIX=\"iOS\"\n    fi\nfi\n\nif [ -n \"$IMG_OS_PREFIX\" ]; then\nrsync -aq --exclude \".svn\" --exclude \".DS_Store\" \"$SRC_DIR/images/$IMG_OS_PREFIX/\" \"$DST_DIR/images\"\nfi\n\ncd \"$SRC_DIR\"\ncat controls/*.css | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.css\"\ncat `cat js-files` | perl -0777 -pe 's{/\\*.*?\\*/}{}gs' > \"$DST_DIR/modern-media-controls.js\"\n";

NIT: Does removing the `perl -0777 -pe 's{/\\*.*?\\*/}{}gs'` mean that comments are not stripped anymore?  I guess this won't be an issue though if we minify the JS files later anyways.

> Source/WebCore/html/HTMLMediaElement.cpp:7151
> +    Vector<String, 3> mediaControlsScripts = RenderTheme::singleton().mediaControlsScripts();

`auto`?

> Source/WebCore/html/HTMLMediaElement.cpp:7171
> +                scope.clearException();

Can we log the reason that the evaluate failed?  This shouldn't ever happen (i.e. the media controls script shouldn't have any errors), so it'd be nice to have in a sysdiagnose.

[Yusuke Suzuki]

Created attachment 423529 [details]
Patch

[Yusuke Suzuki]

Created attachment 423538 [details]
Patch

[Yusuke Suzuki]

Created attachment 423539 [details]
Patch

[Yusuke Suzuki]

Committed r274607 (235444@main): <https://commits.webkit.org/235444@main>