Bug 55097 - fastMalloc(0) should not be supported
Summary: fastMalloc(0) should not be supported
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-23 15:48 PST by Eric Seidel (no email)
Modified: 2012-06-12 02:38 PDT (History)
8 users (show)

See Also:


Attachments
wip (8.53 KB, patch)
2011-02-23 15:50 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
another wip (11.33 KB, patch)
2011-06-05 17:52 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (12.41 KB, patch)
2011-06-06 15:27 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2011-02-23 15:48:28 PST
fastMalloc(0) should not be supported
Comment 1 Eric Seidel (no email) 2011-02-23 15:50:33 PST
Created attachment 83569 [details]
wip
Comment 2 Eric Seidel (no email) 2011-02-23 15:51:27 PST
The idea for this patch came from finding and fixing bug 55091.
Comment 3 Adam Barth 2011-02-23 15:56:50 PST
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.
Comment 4 Eric Seidel (no email) 2011-02-23 16:01:29 PST
(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.
Comment 5 Adam Barth 2011-02-23 16:18:25 PST
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.
Comment 6 Anders Carlsson 2011-02-23 16:36:58 PST
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()) ?
Comment 7 Darin Adler 2011-02-23 16:51:08 PST
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.
Comment 8 Alexey Proskuryakov 2011-02-23 22:12:49 PST
+    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.
Comment 9 Eric Seidel (no email) 2011-06-05 11:11:32 PDT
Updating the patch now.
Comment 10 Alexey Proskuryakov 2011-06-05 12:52:45 PDT
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.
Comment 11 Eric Seidel (no email) 2011-06-05 13:06:05 PDT
(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).
Comment 12 Alexey Proskuryakov 2011-06-05 13:08:51 PDT
Thanks Eric.
Comment 13 Eric Seidel (no email) 2011-06-05 14:45:43 PDT
(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
Comment 14 Alexey Proskuryakov 2011-06-05 15:15:35 PDT
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.
Comment 15 Darin Adler 2011-06-05 17:25:29 PDT
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.
Comment 16 Eric Seidel (no email) 2011-06-05 17:52:05 PDT
Created attachment 96056 [details]
another wip
Comment 17 Eric Seidel (no email) 2011-06-06 15:27:57 PDT
Created attachment 96139 [details]
Patch
Comment 18 Eric Seidel (no email) 2011-06-06 15:38:46 PDT
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).
Comment 19 Alexey Proskuryakov 2011-06-06 15:47:31 PDT
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?
Comment 20 Alexey Proskuryakov 2011-06-06 15:47:47 PDT
comment 14
Comment 21 Yong Li 2011-06-10 08:52:13 PDT
Excuse me, but is there any test results showing how much performance gain can be given by this patch?
Comment 22 Adam Barth 2011-10-14 22:49:39 PDT
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.