Bug 186969 – Remove static initializers more

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Remove static initializers more

[Yusuke Suzuki]

Created attachment 343433 [details]
Patch

[Yusuke Suzuki]

Created attachment 343436 [details]
Patch

[Yusuke Suzuki]

Created attachment 343437 [details]
Patch

[Yusuke Suzuki]

Created attachment 343492 [details]
Patch

[Michael Catanzaro]

Comment on attachment 343492 [details]
Patch

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

Yes, exciting patch!

I'm not sure about the ResourceUsageData change... the code was surely easier to read the way it was before. Does your change make it a trivially destructible type? If so, then I suppose it is good, to avoid static initializers and destructors.

> Source/WebCore/ChangeLog:8
> +        This patch removes static initializers more. They typically exists in GTK port.

Yes, static initializers are very tempting, but we should really avoid them.

We just added one in RenderThemeGtk, in fact, that should really have used NeverDestroyed instead. (CC Carlos Eduardo ;)

> Source/WebCore/ChangeLog:30
> +        * platform/network/soup/SoupNetworkSession.cpp:

I swear I did this just yesterday, as part of an effort to reduce asan warnings, then gave up and threw away the work when I noticed the next issue (PasteboardHelper) and assumed there would be an unending number of them. Anyway, just an interesting coincidence to note.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:40
> -const static CapabilityValueOrRange defaultVolumeCapability = CapabilityValueOrRange(0.0, 1.0);
> +static CapabilityValueOrRange defaultVolumeCapability()
> +{
> +    return CapabilityValueOrRange(0.0, 1.0);
> +}

Of course the difference is that now a new CapabilityValueOrRange will be constructed every time this function is called. I guess that's cheap?

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:58
>  static bool gIgnoreTLSErrors;
> -static CString gInitialAcceptLanguages;
> -static SoupNetworkProxySettings gProxySettings;
> +static CString& initialAcceptLanguages()
> +{
> +    static NeverDestroyed<CString> storage;
> +    return storage.get();
> +}
> +static SoupNetworkProxySettings proxySettings()
> +{
> +    static NeverDestroyed<SoupNetworkProxySettings> settings;
> +    return settings.get();
> +}
>  static GType gCustomProtocolRequestType;

How about some blank lines above and below the functions? And move gCustomProtocolRequestType up beneath gIgnoreTLSErrors.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:53
> +static WTF::Vector<unsigned>& listenerIds()
> +{
> +    static NeverDestroyed<WTF::Vector<unsigned>> ids;
> +    return ids.get();
> +}
> +static NotificationHandlersMap& notificationHandlers()
> +{
> +    static NeverDestroyed<NotificationHandlersMap> map;
> +    return map.get();
> +}
>  AccessibilityNotificationHandler* globalNotificationHandler = nullptr;

Again, I think it's easier to read when there are more blank lines!

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Yes, static initializers are very tempting, but we should really avoid them.
> 
> We just added one in RenderThemeGtk, in fact, that should really have used
> NeverDestroyed instead. (CC Carlos Eduardo ;)

OK, that patch has not landed yet. I've added a comment to bug #126907 to avoid it.

[EWS Watchlist]

Comment on attachment 343492 [details]
Patch

Attachment 343492 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8338852

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 343555 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 343492 [details]
Patch

Attachment 343492 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8339576

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 343566 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Yusuke Suzuki]

Comment on attachment 343492 [details]
Patch

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

Thanks!

>> Source/WebCore/ChangeLog:8
>> +        This patch removes static initializers more. They typically exists in GTK port.
> 
> Yes, static initializers are very tempting, but we should really avoid them.
> 
> We just added one in RenderThemeGtk, in fact, that should really have used NeverDestroyed instead. (CC Carlos Eduardo ;)

Yeah, removing static initializers makes the load of the library safer and avoids bunch of crazy issues caused by the order of initializations of these objects.

>> Source/WebCore/ChangeLog:30
>> +        * platform/network/soup/SoupNetworkSession.cpp:
> 
> I swear I did this just yesterday, as part of an effort to reduce asan warnings, then gave up and threw away the work when I noticed the next issue (PasteboardHelper) and assumed there would be an unending number of them. Anyway, just an interesting coincidence to note.

Currently, we have one more static initializer in WebKitGTK+: XErrorTrapper in PluginProcessMainUnix. Personally, I think we can make it safer by introducing a singleton for collecting errors. But idk for now.

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:40
>> +}
> 
> Of course the difference is that now a new CapabilityValueOrRange will be constructed every time this function is called. I guess that's cheap?

Yeah, it's really cheap. I think we can further make it cheap by introducing `constexpr` constructor for CapabilityValueOrRange and using `const static constexpr CapabilityValueOrRange defaultVolumeCapability { ... };`.
But right now, I take a simpler approach. It would be nice if we can introduce `constexpr` constructor in the future.

>> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:58
>>  static GType gCustomProtocolRequestType;
> 
> How about some blank lines above and below the functions? And move gCustomProtocolRequestType up beneath gIgnoreTLSErrors.

Looks nice. Added and changed.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:53
>>  AccessibilityNotificationHandler* globalNotificationHandler = nullptr;
> 
> Again, I think it's easier to read when there are more blank lines!

Fixed.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #5)
> I'm not sure about the ResourceUsageData change... the code was surely
> easier to read the way it was before. Does your change make it a trivially
> destructible type? If so, then I suppose it is good, to avoid static
> initializers and destructors.

Yes, this patch makes ResourceUsageData trivially constructible by defining constexpr constructor, which is also a default constructor.
And our copy constructor is not necessary since the default copy constructor works well for this case.

[Yusuke Suzuki]

Committed r233239: <https://trac.webkit.org/changeset/233239>

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #11) 
> Currently, we have one more static initializer in WebKitGTK+: XErrorTrapper
> in PluginProcessMainUnix. Personally, I think we can make it safer by
> introducing a singleton for collecting errors. But idk for now.

Mmm, I see that one is more complicated... would be good to report a bug for that.

[Radar WebKit Bug Importer]

<rdar://problem/41502857>