Bug 200611 - [WTF][JSC] Make JSC and WTF aggressively-fast-malloced
Summary: [WTF][JSC] Make JSC and WTF aggressively-fast-malloced
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks: 200620
  Show dependency treegraph
 
Reported: 2019-08-10 02:18 PDT by Yusuke Suzuki
Modified: 2019-09-05 13:05 PDT (History)
10 users (show)

See Also:


Attachments
Patch (161.07 KB, patch)
2019-08-10 02:33 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.52 KB, patch)
2019-08-10 02:44 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (166.46 KB, patch)
2019-08-10 02:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (165.16 KB, patch)
2019-08-10 03:04 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (165.82 KB, patch)
2019-08-10 19:44 PDT, Yusuke Suzuki
sbarati: review+
ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-08-10 02:18:37 PDT
[WTF][JSC] Make JSC and WTF aggressively-fast-malloced
Comment 1 Yusuke Suzuki 2019-08-10 02:33:12 PDT
Created attachment 376005 [details]
Patch
Comment 2 Build Bot 2019-08-10 02:36:38 PDT
Attachment 376005 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 188 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Yusuke Suzuki 2019-08-10 02:44:59 PDT
Created attachment 376006 [details]
Patch
Comment 4 Build Bot 2019-08-10 02:46:51 PDT
Attachment 376006 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 195 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Yusuke Suzuki 2019-08-10 02:48:50 PDT
Created attachment 376007 [details]
Patch
Comment 6 Build Bot 2019-08-10 02:52:06 PDT
Attachment 376007 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 195 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yusuke Suzuki 2019-08-10 03:04:10 PDT
Created attachment 376008 [details]
Patch
Comment 8 Build Bot 2019-08-10 03:06:18 PDT
Attachment 376008 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 193 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2019-08-10 05:38:52 PDT
Comment on attachment 376008 [details]
Patch

Attachment 376008 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12890469

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint
Comment 10 Yusuke Suzuki 2019-08-10 19:44:54 PDT
Created attachment 376026 [details]
Patch
Comment 11 Build Bot 2019-08-10 19:48:00 PDT
Attachment 376026 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Optional.h:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Optional.h:585:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, memory corruption  [changelog/unwantedsecurityterms] [3]
Total errors found: 3 in 194 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Build Bot 2019-08-10 22:02:17 PDT
Comment on attachment 376026 [details]
Patch

Attachment 376026 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12892898

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
Comment 13 Saam Barati 2019-08-12 11:41:41 PDT
Comment on attachment 376026 [details]
Patch

rs=me
Comment 14 Mark Lam 2019-08-12 12:16:20 PDT
Comment on attachment 376026 [details]
Patch

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

> Source/WTF/wtf/DoublyLinkedList.h:69
> +class DoublyLinkedList {

I know we normally embed this in another class, but if any client chooses to malloc this, why not also make this FastMalloc'ed?
Comment 15 Yusuke Suzuki 2019-08-12 13:56:00 PDT
Comment on attachment 376026 [details]
Patch

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

>> Source/WTF/wtf/DoublyLinkedList.h:69
>> +class DoublyLinkedList {
> 
> I know we normally embed this in another class, but if any client chooses to malloc this, why not also make this FastMalloc'ed?

If we inherit this and RefCounted<T>, it poses ambiguous `new` definitions, and we need to put `WTF_MAKE_FAST_ALLOCATED` to the derived class again. And I do not wan to do that in this patch.
So this patch avoids annotating relatively-interface-like classes.
I'll later add makeUnique, which always has `static_assert` to check whether the class T is FastMalloced one.
I think this `makeUnique` and `RefCounted` can cover 100% of our use case, and both can ensure that all the classes are having FastMalloc annotation.
So, even if we do not annotate it with WTF_MAKE_FAST_ALLOCATED, later we still can find the classes that do not have WTF_MAKE_FAST_ALLOCATED by compile error.
Comment 16 Yusuke Suzuki 2019-08-12 13:57:28 PDT
Committed r248546: <https://trac.webkit.org/changeset/248546>
Comment 17 Radar WebKit Bug Importer 2019-08-12 13:58:20 PDT
<rdar://problem/54224528>
Comment 18 Darin Adler 2019-08-12 16:26:09 PDT
Here’s something else to consider for the future: If we create a WTF::makeUnique function, it does not need to check for WTF_MAKE_FAST_ALLOCATED. Instead it can have code that uses the fast allocator regardless of whether the class does WTF_MAKE_FAST_ALLOCATED. Rather than helping us sprinkle WTF_MAKE_FAST_ALLOCATED everywhere, it could reduce the importance of it. Then we would only need to use WTF_MAKE_FAST_ALLOCATED for classes and structs that need to use the fast allocator when *not* calling WTF::makeUnique.
Comment 19 Yusuke Suzuki 2019-08-12 23:20:48 PDT
(In reply to Darin Adler from comment #18)
> Here’s something else to consider for the future: If we create a
> WTF::makeUnique function, it does not need to check for
> WTF_MAKE_FAST_ALLOCATED. Instead it can have code that uses the fast
> allocator regardless of whether the class does WTF_MAKE_FAST_ALLOCATED.
> Rather than helping us sprinkle WTF_MAKE_FAST_ALLOCATED everywhere, it could
> reduce the importance of it. Then we would only need to use
> WTF_MAKE_FAST_ALLOCATED for classes and structs that need to use the fast
> allocator when *not* calling WTF::makeUnique.

Nice catch. After considering about this idea, personally, I like annotating classes with WTF_MAKE_FAST_ALLOCATED. Let me explain why I think so.

Since default deleter is performing `delete` and this is not what we want for FastMalloced ones, we need to return std::unique_ptr<T, FastFreeDeleter> for T if T does not have FastMalloc-annotation. To me, this sounds a bit dangerous. The dangerous example is the following.

auto pointer = WTF::makeUnique<T>();
// Super dangerous, but sometimes it is required...
auto* rawPointer = pointer.release();
// Passing rawPointer to somewhere, and
delete rawPointer;

The above one becomes invalid because pointer may start requiring non `delete` destroying function.
In the above case, the correct way becomes the following.

rawPointer->~T();
fastFree(rawPointer);

This looks non-intuitive. And having two ways to destroying objects (`delete` or the above one) can be error-prone.
If we have WTF_MAKE_FAST_ALLOCATED for T, we do not need to care about this. "new" and "delete" operators are defined, and C++ way works.
The simple invariant, "makeUnique just does `new` internally. And `delete` operator does `delete`. default deleter is just doing `delete`", is kept.


While we need to annotate many classes with WTF_MAKE_FAST_ALLOCATED, it is one time cost when we add a class.
And, by introducing `WTF::makeUnique<>`, we no longer forget adding this. So, personally I think just implementing `makeUnique` as follows looks simple :)

makeUnique(...)
    static_assert(T is FastMalloced or IsoHeaped);
    return make_unique<T>(...)
Comment 20 Yusuke Suzuki 2019-08-13 11:54:38 PDT
Committed r248603: <https://trac.webkit.org/changeset/248603>
Comment 21 Antti Koivisto 2019-08-23 09:51:44 PDT
> Since default deleter is performing `delete` and this is not what we want
> for FastMalloced ones, we need to return std::unique_ptr<T, FastFreeDeleter>
> for T if T does not have FastMalloc-annotation. To me, this sounds a bit
> dangerous. The dangerous example is the following.

What is dangerous about this? It should reliably crash, right?
Comment 22 Yusuke Suzuki 2019-08-23 11:41:25 PDT
(In reply to Antti Koivisto from comment #21)
> > Since default deleter is performing `delete` and this is not what we want
> > for FastMalloced ones, we need to return std::unique_ptr<T, FastFreeDeleter>
> > for T if T does not have FastMalloc-annotation. To me, this sounds a bit
> > dangerous. The dangerous example is the following.
> 
> What is dangerous about this? It should reliably crash, right?

It depends on how the malloc/free is implemented. For example, in bmalloc, it does not crash immediately since it goes to deallocation-logs until it is flushed.
Comment 23 Said Abou-Hallawa 2019-08-23 14:26:44 PDT
Comment on attachment 376026 [details]
Patch

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

> Source/WTF/ChangeLog:695
> +        (WTF::BinarySemaphore::waitFor): Deleted.
> +        (WTF::BinarySemaphore::wait): Deleted.

These "Deleted" lines and the ones above them are wrong. Your change did not delete any function in WTF sources. Can you please clean up this change log? There might be a bug in Tools/Scripts/prepare-ChangeLog.
Comment 24 Yusuke Suzuki 2019-08-23 15:17:48 PDT
Comment on attachment 376026 [details]
Patch

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

>> Source/WTF/ChangeLog:695
>> +        (WTF::BinarySemaphore::wait): Deleted.
> 
> These "Deleted" lines and the ones above them are wrong. Your change did not delete any function in WTF sources. Can you please clean up this change log? There might be a bug in Tools/Scripts/prepare-ChangeLog.

Checked, and seems that our prepare-ChangeLog had a bug, annotating all the methods "Deleted" if the class is annotated with "final". Filed https://bugs.webkit.org/show_bug.cgi?id=201093.
Comment 25 Yusuke Suzuki 2019-08-23 15:20:03 PDT
Comment on attachment 376026 [details]
Patch

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

>>> Source/WTF/ChangeLog:695
>>> +        (WTF::BinarySemaphore::wait): Deleted.
>> 
>> These "Deleted" lines and the ones above them are wrong. Your change did not delete any function in WTF sources. Can you please clean up this change log? There might be a bug in Tools/Scripts/prepare-ChangeLog.
> 
> Checked, and seems that our prepare-ChangeLog had a bug, annotating all the methods "Deleted" if the class is annotated with "final". Filed https://bugs.webkit.org/show_bug.cgi?id=201093.

And filed https://bugs.webkit.org/show_bug.cgi?id=201094
Comment 26 Geoffrey Garen 2019-09-04 21:29:43 PDT
> Nice catch. After considering about this idea, personally, I like annotating
> classes with WTF_MAKE_FAST_ALLOCATED. Let me explain why I think so.
> 
> Since default deleter is performing `delete` and this is not what we want
> for FastMalloced ones, we need to return std::unique_ptr<T, FastFreeDeleter>
> for T if T does not have FastMalloc-annotation. To me, this sounds a bit
> dangerous. The dangerous example is the following.
> 
> auto pointer = WTF::makeUnique<T>();
> // Super dangerous, but sometimes it is required...
> auto* rawPointer = pointer.release();
> // Passing rawPointer to somewhere, and
> delete rawPointer;

Technically, it is always incorrect C++ to call "delete ptr.release()". The correct way to manually delete a unique_ptr is to call ptr.get_deleter(ptr.release()).

So, this concern is really an objection against using unique_ptr's custom deleter feature in any context, FastMalloc or otherwise.

I wonder: Is it common in our code to "delete ptr.release()"? If it isn't common, or if our GuardMalloc and Asan bots would catch the error immediately, maybe the danger is acceptable.

(This discussion reminds me of the problem of calling "new" followed by "free" instead of "delete". That problem has happened in WebKit from time to time. I don't think we've ever come up with a reliable solution, other than to avoid manual use of "free" and "delete" entirely.)
Comment 27 Geoffrey Garen 2019-09-04 21:33:01 PDT
> The correct way to manually delete a unique_ptr is to call
> ptr.get_deleter(ptr.release()).

ptr.get_deleter()(ptr.release())
Comment 28 Yusuke Suzuki 2019-09-04 22:01:44 PDT
(In reply to Geoffrey Garen from comment #26)
> > Nice catch. After considering about this idea, personally, I like annotating
> > classes with WTF_MAKE_FAST_ALLOCATED. Let me explain why I think so.
> > 
> > Since default deleter is performing `delete` and this is not what we want
> > for FastMalloced ones, we need to return std::unique_ptr<T, FastFreeDeleter>
> > for T if T does not have FastMalloc-annotation. To me, this sounds a bit
> > dangerous. The dangerous example is the following.
> > 
> > auto pointer = WTF::makeUnique<T>();
> > // Super dangerous, but sometimes it is required...
> > auto* rawPointer = pointer.release();
> > // Passing rawPointer to somewhere, and
> > delete rawPointer;
> 
> Technically, it is always incorrect C++ to call "delete ptr.release()". The
> correct way to manually delete a unique_ptr is to call
> ptr.get_deleter(ptr.release()).
> 
> So, this concern is really an objection against using unique_ptr's custom
> deleter feature in any context, FastMalloc or otherwise.
> 
> I wonder: Is it common in our code to "delete ptr.release()"? If it isn't
> common, or if our GuardMalloc and Asan bots would catch the error
> immediately, maybe the danger is acceptable.

I think this is particularly common in WebCore/rendering.

WebCore/rendering/RenderBox.cpp

2123 std::unique_ptr<InlineElementBox> RenderBox::createInlineBox()
2124 {
2125     return makeUnique<InlineElementBox>(*this);
2126 }

WebCore/rendering/ComplexLineLayout.cpp

 163     if (is<RenderBox>(*renderer)) {
 164         // FIXME: This is terrible. This branch returns an *owned* pointer!
 165         return downcast<RenderBox>(*renderer).createInlineBox().release();
 166     }

 971 void ComplexLineLayout::removeInlineBox(BidiRun& run, const RootInlineBox& rootLineBox) const
 972 {
 973     auto* inlineBox = run.box();
 974 #if !ASSERT_DISABLED
 975     auto* inlineParent = inlineBox->parent();
 976     while (inlineParent && inlineParent != &rootLineBox) {
 977         ASSERT(!inlineParent->isDirty());
 978         inlineParent = inlineParent->parent();
 979     }
 980     ASSERT(!rootLineBox.isDirty());
 981 #endif
 982     auto* parent = inlineBox->parent();
 983     inlineBox->removeFromParent();
 984
 985     auto& renderer = run.renderer();
 986     if (is<RenderText>(renderer))
 987         downcast<RenderText>(renderer).removeTextBox(downcast<InlineTextBox>(*inlineBox));
 988     delete inlineBox;

WebCore/rendering/RenderLineBoxList.cpp

 52 void RenderLineBoxList::appendLineBox(std::unique_ptr<InlineFlowBox> box)
 53 {
 54     checkConsistency();
 55
 56     InlineFlowBox* boxPtr = box.release();
 57
 58     if (!m_firstLineBox) {
 59         m_firstLineBox = boxPtr;
 60         m_lastLineBox = boxPtr;
 61     } else {
 62         m_lastLineBox->setNextLineBox(boxPtr);
 63         boxPtr->setPreviousLineBox(m_lastLineBox);
 64         m_lastLineBox = boxPtr;
 65     }
 66
 67     checkConsistency();
 68 }

133 void RenderLineBoxList::deleteLineBoxes()
134 {
135     if (m_firstLineBox) {
136         InlineFlowBox* next;
137         for (InlineFlowBox* curr = m_firstLineBox; curr; curr = next) {
138             next = curr->nextLineBox();
139             delete curr;
140         }
141         m_firstLineBox = nullptr;
142         m_lastLineBox = nullptr;
143     }
144 }


> (This discussion reminds me of the problem of calling "new" followed by
> "free" instead of "delete". That problem has happened in WebKit from time to
> time. I don't think we've ever come up with a reliable solution, other than
> to avoid manual use of "free" and "delete" entirely.)

Yeah, I agree that removing manual `delete` is always better direction BTW.
Comment 29 Geoffrey Garen 2019-09-05 09:37:29 PDT
> > I wonder: Is it common in our code to "delete ptr.release()"? If it isn't
> > common, or if our GuardMalloc and Asan bots would catch the error
> > immediately, maybe the danger is acceptable.
> 
> I think this is particularly common in WebCore/rendering.

Maybe if Antti removes the manual calls to delete in WebCore/rendering, we can reconsider his suggestion. :P
Comment 30 Antti Koivisto 2019-09-05 10:40:57 PDT
> > I think this is particularly common in WebCore/rendering.
> 
> Maybe if Antti removes the manual calls to delete in WebCore/rendering, we
> can reconsider his suggestion. :P

Stuff under rendering/ is ISO allocated so would probably retain class annotations in any case.
Comment 31 Yusuke Suzuki 2019-09-05 11:22:07 PDT
(In reply to Antti Koivisto from comment #30)
> > > I think this is particularly common in WebCore/rendering.
> > 
> > Maybe if Antti removes the manual calls to delete in WebCore/rendering, we
> > can reconsider his suggestion. :P
> 
> Stuff under rendering/ is ISO allocated so would probably retain class
> annotations in any case.

I think makeUnique<>'s static_assert can ensure this invariant.
`release()` and `delete` pattern looks dangerous and I prefer ensuring fast-malloced in makeUnique<> when we use this pattern.
If we allocate type T automatically from fast-malloc, I think we should have our own unique_ptr which removes `release` and `custom_deleter`, like,

1. Introducing something like `WTF::OwnPtr` which is similar to std::unique_ptr, but it does not have `release()`.
2. Remove std::unique_ptr's `release()` and delete usage. 
2. Replacing `makeUnique` with `makeOwn` which returns OwnPtr using fast-malloc automatically.
Comment 32 Yusuke Suzuki 2019-09-05 13:05:31 PDT
BTW, note that

1. XPath related code
2. BlacklistUpdater

also use this pattern in WebCore at least.