[WTF][JSC] Make JSC and WTF aggressively-fast-malloced

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[WTF][JSC] Make JSC and WTF aggressively-fast-malloced

[Yusuke Suzuki]

Created attachment 376005 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 376006 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 376007 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 376008 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 376026 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Saam Barati]

Comment on attachment 376026 [details]
Patch

rs=me

[Mark Lam]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r248546: <https://trac.webkit.org/changeset/248546>

[Radar WebKit Bug Importer]

<rdar://problem/54224528>

[Darin Adler]

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.

[Yusuke Suzuki]

(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>(...)

[Yusuke Suzuki]

Committed r248603: <https://trac.webkit.org/changeset/248603>

[Antti Koivisto]

> 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?

[Yusuke Suzuki]

(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.

[Said Abou-Hallawa]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[Geoffrey Garen]

> 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.)

[Geoffrey Garen]

> The correct way to manually delete a unique_ptr is to call
> ptr.get_deleter(ptr.release()).

ptr.get_deleter()(ptr.release())

[Yusuke Suzuki]

(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.

[Geoffrey Garen]

> > 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

[Antti Koivisto]

> > 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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

BTW, note that

1. XPath related code
2. BlacklistUpdater

also use this pattern in WebCore at least.