Bug 200973 – [WPE] Build failure after r248846 ([WTF] Add makeUnique<T>, which ensures T is fast-allocated, makeUnique / makeUniqueWithoutFastMallocCheck part)

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Pablo Saavedra]

WPE fails to build with `-DENABLE_WPE_QT_API=ON` after changes done by r248846 in:

* Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp
* Source/WebKit/UIProcess/API/wpe/qt/WPEQtViewBackend.cpp

; to forces FastMalloc use.


Related to:

* https://bugs.webkit.org/show_bug.cgi?id=200611
* https://bugs.webkit.org/show_bug.cgi?id=200620

[Pablo Saavedra]

Created attachment 376867 [details]
patch

[EWS Watchlist]

Attachment 376867 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:30:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: memory corruption  [changelog/unwantedsecurityterms] [3]
ERROR: Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:442:  Use 'WTF::makeUnique<>' instead of 'std::make_unique<>'.  [runtime/wtf_make_unique] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 376867 [details]
patch

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

> Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:442
> -    std::unique_ptr<JavascriptCallbackData> data = makeUnique<JavascriptCallbackData>(callback, QPointer<WPEQtView>(this));
> +    std::unique_ptr<JavascriptCallbackData> data = std::make_unique<JavascriptCallbackData>(callback, QPointer<WPEQtView>(this));

This doesn't look right. I think you can simply make JavascriptCallbackData WTF_MAKE_FAST_ALLOCATED and then makeUnique would work?

[Pablo Saavedra]

Created attachment 376869 [details]
patch

[Pablo Saavedra]

(In reply to Philippe Normand from comment #3)
> Comment on attachment 376867 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376867&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:442
> > -    std::unique_ptr<JavascriptCallbackData> data = makeUnique<JavascriptCallbackData>(callback, QPointer<WPEQtView>(this));
> > +    std::unique_ptr<JavascriptCallbackData> data = std::make_unique<JavascriptCallbackData>(callback, QPointer<WPEQtView>(this));
> 
> This doesn't look right. I think you can simply make JavascriptCallbackData
> WTF_MAKE_FAST_ALLOCATED and then makeUnique would work?

Faster than my eyes, I uploaded the right version one minute ago.  Sorry for the noise with the first one.

[EWS Watchlist]

Attachment 376869 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:30:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 376869 [details]
patch

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

> Source/WebKit/ChangeLog:31
> +            [WTF][JSC] Make JSC and WTF aggressively-fast-malloced
> +            https://bugs.webkit.org/show_bug.cgi?id=200611
> +            ...
> +            Putting WebKit classes in FastMalloc has many benefits.
> +
> +            1. Simply, it is fast.
> +            2. vmmap can tell the amount of memory used for WebKit.
> +            3. bmalloc can isolate WebKit memory allocation from the rest of
> +               the world. This is useful since we can know more about what
> +               component is corrupting the memory from the memory corruption
> +               crash.

Can you remove this? It would make the style bot happy and we can land this through cq.

[Pablo Saavedra]

Created attachment 376871 [details]
patch

[Pablo Saavedra]

(In reply to Philippe Normand from comment #7)
> Comment on attachment 376869 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376869&action=review
> 
> > Source/WebKit/ChangeLog:31
> > +            [WTF][JSC] Make JSC and WTF aggressively-fast-malloced
> > +            https://bugs.webkit.org/show_bug.cgi?id=200611
> > +            ...
> > +            Putting WebKit classes in FastMalloc has many benefits.
> > +
> > +            1. Simply, it is fast.
> > +            2. vmmap can tell the amount of memory used for WebKit.
> > +            3. bmalloc can isolate WebKit memory allocation from the rest of
> > +               the world. This is useful since we can know more about what
> > +               component is corrupting the memory from the memory corruption
> > +               crash.
> 
> Can you remove this? It would make the style bot happy and we can land this
> through cq.

done and thanks for pointing the solution.

[Philippe Normand]

Comment on attachment 376871 [details]
patch

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

> Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:383
> +    WTF_MAKE_FAST_ALLOCATED;

Sorry, I've just found out there's a WTF_MAKE_STRUCT_FAST_ALLOCATED macro as well. Can you use it here?

[Pablo Saavedra]

Created attachment 376874 [details]
patch

[Pablo Saavedra]

(In reply to Philippe Normand from comment #10)
> Comment on attachment 376871 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376871&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/qt/WPEQtView.cpp:383
> > +    WTF_MAKE_FAST_ALLOCATED;
> 
> Sorry, I've just found out there's a WTF_MAKE_STRUCT_FAST_ALLOCATED macro as
> well. Can you use it here?

Sure ... done

[Philippe Normand]

Comment on attachment 376874 [details]
patch

Thanks! :)

[WebKit Commit Bot]

Comment on attachment 376874 [details]
patch

Clearing flags on attachment: 376874

Committed r248941: <https://trac.webkit.org/changeset/248941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.