Bug 121883 – [Windows] Refactor RuntimeEnabledFeatures as a Singleton to prevent crashes caused by cross-DLL boundary access

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

The RuntimeEnabledFeatures pseudo-singleton class has a number of inline getters and setters. When these are used to access the static members of the class across a DLL boundary, we are encountering crashes under Windows.

While this seems to be behavior specific to Visual Studio, it is not a good design and should be refactored to act as a 'real' Singleton object to avoid non-deterministic behavior for reading/writing this memory.

[Brent Fulgham]

Created attachment 212529 [details]
Patch

[Early Warning System Bot]

Comment on attachment 212529 [details]
Patch

Attachment 212529 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2174074

[Early Warning System Bot]

Comment on attachment 212529 [details]
Patch

Attachment 212529 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/2165004

[EFL EWS Bot]

Comment on attachment 212529 [details]
Patch

Attachment 212529 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2162073

[Build Bot]

Comment on attachment 212529 [details]
Patch

Attachment 212529 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2217006

[Build Bot]

Comment on attachment 212529 [details]
Patch

Attachment 212529 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2169077

[kov's GTK+ EWS bot]

Comment on attachment 212529 [details]
Patch

Attachment 212529 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/2174085

[EFL EWS Bot]

Comment on attachment 212529 [details]
Patch

Attachment 212529 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2190067

[Jer Noble]

Comment on attachment 212529 [details]
Patch

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

Awesome. Just a few nits.

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:142
> +RuntimeEnabledFeatures& RuntimeEnabledFeatures::sharedRuntimeEnabledFeatures()

RuntimeEnabledFeatures::sharedRuntimeEnabledFeatures() seems a bit redundant.  How about RuntimeEnabledFeatures::sharedFeatures()?

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:45
>  class RuntimeEnabledFeatures {
>  public:

You should add WTF_MAKE_NONCOPYABLE(RuntimeEnabledFeatures) here now that we have an instance.

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:317
> +    bool isLocalStorageEnabled;
> +    bool isSessionStorageEnabled;
> +    bool isWebkitNotificationsEnabled;
> +    bool isApplicationCacheEnabled;
> +    bool isDataTransferItemsEnabled;
> +    bool isGeolocationEnabled;
> +    bool isIndexedDBEnabled;
> +    bool isTouchEnabled;
> +    bool isDeviceMotionEnabled;
> +    bool isDeviceOrientationEnabled;
> +    bool isSpeechInputEnabled;
> +    bool isCanvasPathEnabled;
> +    bool isCSSExclusionsEnabled;
> +    bool isCSSShapesEnabled;
> +    bool isCSSRegionsEnabled;
> +    bool isCSSCompositingEnabled;
> +    bool isLangAttributeAwareFormControlUIEnabled;

While we're cleaning up bad coding style, we should really make all these variables conform to WebKit Coding Style, with m_ prefixes for each.

[Brent Fulgham]

Created attachment 212601 [details]
Patch

[Brent Fulgham]

Created attachment 212604 [details]
Updated after removal of HTMLContentElement.cpp

[Brent Fulgham]

Once this patch is in place we can reactivate CSS Shapes on Windows (i.e., revert Bug 121879).

[Early Warning System Bot]

Comment on attachment 212604 [details]
Updated after removal of HTMLContentElement.cpp

Attachment 212604 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/2212053

[Early Warning System Bot]

Comment on attachment 212604 [details]
Updated after removal of HTMLContentElement.cpp

Attachment 212604 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/2217054

[Brent Fulgham]

Created attachment 212608 [details]
Patch

[Build Bot]

Comment on attachment 212608 [details]
Patch

Attachment 212608 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2217061

[Brent Fulgham]

Created attachment 212612 [details]
Removed offending WebCore.exp.in entries

[Brent Fulgham]

Comment on attachment 212612 [details]
Removed offending WebCore.exp.in entries

Watching the mac/mac-wk2 bots. They will probably fail one more time to show me the symbol I need to add to WebCore.exp.in to support the new Singleton object.

[Build Bot]

Comment on attachment 212612 [details]
Removed offending WebCore.exp.in entries

Attachment 212612 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2224054

[Build Bot]

Comment on attachment 212612 [details]
Removed offending WebCore.exp.in entries

Attachment 212612 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2220014

[Brent Fulgham]

Created attachment 212620 [details]
Removed typo from WebCore.exp.in

[Build Bot]

Comment on attachment 212620 [details]
Removed typo from WebCore.exp.in

Attachment 212620 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2240010

[Brent Fulgham]

Created attachment 212625 [details]
Updated with missing symbol in WebCore.exp.in

[Jer Noble]

Comment on attachment 212625 [details]
Updated with missing symbol in WebCore.exp.in

r=me

[Brent Fulgham]

Committed r156424: <http://trac.webkit.org/changeset/156424>

[Csaba Osztrogonác]

Comment on attachment 212625 [details]
Updated with missing symbol in WebCore.exp.in

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

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:419
> +#ifndef CLASS_IF_GCC
> +#if COMPILER(GCC)
> +#define CLASS_IF_GCC class
> +#else
> +#define CLASS_IF_GCC
> +#endif
> +#endif
> +    friend CLASS_IF_GCC NeverDestroyed<RuntimeEnabledFeatures>;

Sooooooo ugly. :-/

Is it a well-known MSVC bug? It seems we ran into the same bug in https://bugs.webkit.org/show_bug.cgi?id=123442
But I prefer a general fix for it instead of adding workarounds everywhere.