Bug 55097 – fastMalloc(0) should not be supported

• Component: New Bugs
• Status: NEW

[Eric Seidel (no email)]

fastMalloc(0) should not be supported

[Eric Seidel (no email)]

Created attachment 83569 [details]
wip

[Eric Seidel (no email)]

The idea for this patch came from finding and fixing bug 55091.

[Adam Barth]

Comment on attachment 83569 [details]
wip

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

> Source/WebCore/dom/Document.cpp:1272
> +        return "";

Not String() or emptyString() ?

> Source/WebCore/loader/DocumentWriter.cpp:222
> -    // make sure nothing's left in there
> +    // FIXME: This is intended to flush the decoder.  I'm not sure it's still needed though.
> +    // Certainly we could expose a cleaner way to do this.
>      addData(0, 0, true);

I've wanted to kill this line of code for a while.  I looked into it recently.  It's non-trivial.

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 83569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83569&action=review
> 
> > Source/WebCore/dom/Document.cpp:1272
> > +        return "";
> 
> Not String() or emptyString() ?

I just matched what the rests of that function did.  But I'm happy to change them all to emptyString() or whatever is the current best practice.

[Adam Barth]

It depends whether clients want an empty string or a null string.  In most code, we return String() which is the null string, in cases like that.

[Anders Carlsson]

Comment on attachment 83569 [details]
wip

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

> Source/WebCore/loader/TextResourceDecoder.cpp:689
> +        result = m_codec->decode(m_buffer.data(), m_buffer.size(), true, m_contentType == XML && !m_useLenientXMLDecoding, m_sawError);

if (!m_buffer.isEmpty()) ?

[Darin Adler]

Comment on attachment 83569 [details]
wip

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

>> Source/WebCore/loader/TextResourceDecoder.cpp:689
>> -    String result = m_codec->decode(m_buffer.data(), m_buffer.size(), true, m_contentType == XML && !m_useLenientXMLDecoding, m_sawError);
>> +    String result;
>> +    if (m_buffer.size())
>> +        result = m_codec->decode(m_buffer.data(), m_buffer.size(), true, m_contentType == XML && !m_useLenientXMLDecoding, m_sawError);
> 
> if (!m_buffer.isEmpty()) ?

This change is wrong. Calling the text codec with flush equal to true can do work even if the data passed in has no length.

[Alexey Proskuryakov]

+    ASSERT(n); // fastMalloc(0) returns implementation-specific results and just wastes time.

Could you please elaborate on how the results are implementation specific?

It may or may not be a good idea to forbid fastMalloc(0) - the downsides I can think of is that more branching will be needed in code that could assume a non-null pointer before, and that fastMalloc will be less like malloc, confusingly to people working on the code.

[Eric Seidel (no email)]

Updating the patch now.

[Alexey Proskuryakov]

I think that if you want to make fastMalloc semantically different from malloc in this case, you also need to rename it from fastMalloc to WTF::allocate or something else that doesn't have "malloc" in it.

[Eric Seidel (no email)]

(In reply to comment #10)
> I think that if you want to make fastMalloc semantically different from malloc in this case, you also need to rename it from fastMalloc to WTF::allocate or something else that doesn't have "malloc" in it.

The C Standard says:

7.20.3 Memory management functions

If the size of the space requested is zero, the behavior is implementation defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object.

I think it's valid for our implementation to ASSERT.  We can obviously leave the if (!size) return 0; if in.

Right now on some platforms our fastMalloc returns 0, and others it returns fastMalloc(1).  In all cases, it does unnecessary work. So even if we don't change our behavior, I think we should ASSERT to make callers check size == 0 and possibly avoid lots of work.  In at least bug 55091 avoiding malloc(0) was a big perf win.


I'll post a patch shortly and start a discussion on webkit-dev.  If we make this change, it should be widely agreed upon (hence the soon-to-be-started thread).

[Alexey Proskuryakov]

Thanks Eric.

[Eric Seidel (no email)]

(In reply to comment #11)
> I think it's valid for our implementation to ASSERT.  We can obviously leave the if (!size) return 0; if in.

I meant "if desired" here.  We could mark said if as UNLIKELY() as well.  In any case, we shouldn't hit it unless we're using fastMalloc for code we don't control (which I'm not sure if we are or not).

I also forgot a link to the spec:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

[Alexey Proskuryakov]

See however the C++ standard, section 3.7.3.1.2 about allocation functions: "If the size of the space requested is zero, the value returned shall not be a null pointer value."

Certainly, "new" does not necessarily need to use fastMalloc directly, but there isn't a lot of WebKit code that uses fastMalloc.

[Darin Adler]

It’s clear and not debatable that it’s legal to call malloc(0) and then call free() on the result of malloc(0). The fact that the value returned is undefined and may be a null pointer is not really material, in my opinion.

If we make fastMalloc(0) illegal, then anyone porting code that previously used malloc to use fastMalloc will have to consider whether the value could be 0. The issue is not “code we don’t control” but rather any code we incorporate and change to call fastMalloc.

However, fastMalloc already is different from malloc in one respect. The fastMalloc function crashes if memory is not available. The real malloc is like tryFastMalloc in that it returns 0 if there is no memory available.

The argument for making fastMalloc(0) illegal is that we think that calling it is often a programming mistake, and we think that a runtime assertion is a good way to catch that mistake.

The arguments for making fastMalloc(0) legal are that we may want to port code using malloc without changing the semantic of a zero size and that we think there are code paths that have a size that is sometimes zero and sometimes non-zero where it is cleaner to be able to malloc and free without having to add a special case for zero.

[Eric Seidel (no email)]

Created attachment 96056 [details]
another wip

[Eric Seidel (no email)]

Created attachment 96139 [details]
Patch

[Eric Seidel (no email)]

Two issues prevent us from turning this on for the Mac port.

1.  Safari crashes on exit in some safari-specific code I can't see.  Running inspector/audits/audits-panel-functional.html may make that more likely.
2.  Any use of the Java plugin will cause WebKitPluginHost to crash on this ASSERT (again, closed-source code).

[Alexey Proskuryakov]

Comment on attachment 96139 [details]
Patch

Changes that had to be made to accommodate this mode do not look like they are improving code quality.

Besides, what about my comment 44?

[Alexey Proskuryakov]

comment 14

[Yong Li]

Excuse me, but is there any test results showing how much performance gain can be given by this patch?

[Adam Barth]

Comment on attachment 96139 [details]
Patch

Clearing the review flag for now.  There seems to be some question about whether to make this change more than a discussion about this particular patch.