Bug 200973 - [WPE] Build failure after r248846 ([WTF] Add makeUnique<T>, which ensures T is fast-allocated, makeUnique / makeUniqueWithoutFastMallocCheck part)
Summary: [WPE] Build failure after r248846 ([WTF] Add makeUnique<T>, which ensures T i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pablo Saavedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-21 05:42 PDT by Pablo Saavedra
Modified: 2019-08-21 07:40 PDT (History)
7 users (show)

See Also:


Attachments
patch (4.12 KB, patch)
2019-08-21 05:49 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (3.98 KB, patch)
2019-08-21 06:11 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (3.42 KB, patch)
2019-08-21 06:22 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (3.27 KB, patch)
2019-08-21 06:49 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pablo Saavedra 2019-08-21 05:42:01 PDT
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
Comment 1 Pablo Saavedra 2019-08-21 05:49:31 PDT
Created attachment 376867 [details]
patch
Comment 2 EWS Watchlist 2019-08-21 05:51:03 PDT
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.
Comment 3 Philippe Normand 2019-08-21 06:10:04 PDT
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?
Comment 4 Pablo Saavedra 2019-08-21 06:11:54 PDT
Created attachment 376869 [details]
patch
Comment 5 Pablo Saavedra 2019-08-21 06:12:54 PDT
(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.
Comment 6 EWS Watchlist 2019-08-21 06:13:51 PDT
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.
Comment 7 Philippe Normand 2019-08-21 06:16:15 PDT
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.
Comment 8 Pablo Saavedra 2019-08-21 06:22:04 PDT
Created attachment 376871 [details]
patch
Comment 9 Pablo Saavedra 2019-08-21 06:23:20 PDT
(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.
Comment 10 Philippe Normand 2019-08-21 06:26:09 PDT
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?
Comment 11 Pablo Saavedra 2019-08-21 06:49:27 PDT
Created attachment 376874 [details]
patch
Comment 12 Pablo Saavedra 2019-08-21 06:49:43 PDT
(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
Comment 13 Philippe Normand 2019-08-21 06:57:45 PDT
Comment on attachment 376874 [details]
patch

Thanks! :)
Comment 14 WebKit Commit Bot 2019-08-21 07:40:02 PDT
Comment on attachment 376874 [details]
patch

Clearing flags on attachment: 376874

Committed r248941: <https://trac.webkit.org/changeset/248941>
Comment 15 WebKit Commit Bot 2019-08-21 07:40:05 PDT
All reviewed patches have been landed.  Closing bug.