Bug 30977 – [Wx] Don't use global operators new/delete in wx port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit wx

[Vadim Zeitlin]

Using global operator new and delete defined in JavaScriptCore/wtf/FastMalloc.h in wxWebKit results in crashes because fastFree() (used by operator delete) can be called for pointers not allocated with fastMalloc() whenever something is allocated inside wx itself and then is freed inside the application code. This is extremely nasty especially when this happens implicitly by using a wx class which happens to call delete from an inlined function: there are no calls to "delete" anywhere in the code at all and it works just fine in debug build (where the function is not inlined) but crashes in release. And while it should be possible to fix this problem otherwise, e.g. by defining more overloads of global delete for wx classes which wouldn't call fastFree() I think it's still too dangerous to keep this delete overload as it's impossible to detect when it is used incorrectly at compile-time so any (inevitable) bugs in such workarounds would be only found when they result in run-time crashes. I was lucky that it crashed directly when opening the page as otherwise I might not have noticed this bug at all during testing.

So I think it's preferable to avoid this problem once and for all by just not using this allocator by default. As it was already done for Qt port (for the same reasons, I guess, although the comment is silent about it), I guess it shouldn't be that bad performance-wise but please correct me if I'm wrong.

Thanks in advance!

[Vadim Zeitlin]

Created attachment 42256 [details]
Patch disabling global operators new and delete for wx port too

Sorry for not attaching the patch when creating the ticket (in fact I did but forgot to enter description and bugzilla decided to create the bug without attachment instead of failing to create it at all...).

[Kevin Ollivier]

Could you provide some specific information about these crashes and how to trigger them? I'd like to take a closer look at this problem and see what our options are. After some googling I found that the QtWebKit devs did seem to have similar reasons for switching away, but they also mentioned that on Windows there's a significant performance increase (30%) when using the TCMalloc-based approach.

http://lists.macosforge.org/pipermail/webkit-dev/2008-September/004912.html

That number is big enough that it makes me wonder if it's not worth considering alternatives. Also, considering that FastMalloc predates the Windows implementation, or any other WebKit ports for that matter, I suspect there may be significant gains on Mac as well.

[Vadim Zeitlin]

The crashes which I got were due to the use of mb_str(): in wx 2.9 it returns a wxScopedCharTypeBuffer whose dtor is inlined and calls DecRef() which is inlined too (all this from reading the disassembly so it's a fact, not just a hypothesis) and so "delete m_data" statement inside DecRef() uses the overloaded global delete operator. But, of course, the m_data pointer was allocated inside wx itself and not with fast malloc overload of new operator so there is a mismatch and it crashes.

FWIW this particular problem is fixed now as I simply removed mb_str() in bug 30980 but others might remain and the worst is that you get a guaranteed crash which can't be detected at compile-time whenever you happen to delete anything allocated by wx in wxWebKit code, even if it happens in an implicit and roundabout way as above. So it would be extremely dangerous to not disable these operators for wx as plenty of stuff allocated by wx may be destroyed from user code.

Concerning the performance, the real solution would be to define operators new/delete using fast malloc for the classes that need them. Defining them for "void *" is a poor idea in general IMO and a catastrophic one in case of wx (and Qt, as it seems).

[Kevin Ollivier]

(In reply to comment #3)
> The crashes which I got were due to the use of mb_str(): in wx 2.9 it returns a
> wxScopedCharTypeBuffer whose dtor is inlined and calls DecRef() which is
> inlined too (all this from reading the disassembly so it's a fact, not just a
> hypothesis) and so "delete m_data" statement inside DecRef() uses the
> overloaded global delete operator. But, of course, the m_data pointer was
> allocated inside wx itself and not with fast malloc overload of new operator so
> there is a mismatch and it crashes.
> 
> FWIW this particular problem is fixed now as I simply removed mb_str() in bug
> 30980 but others might remain and the worst is that you get a guaranteed crash
> which can't be detected at compile-time whenever you happen to delete anything
> allocated by wx in wxWebKit code, even if it happens in an implicit and
> roundabout way as above. So it would be extremely dangerous to not disable
> these operators for wx as plenty of stuff allocated by wx may be destroyed from
> user code.
> 
> Concerning the performance, the real solution would be to define operators
> new/delete using fast malloc for the classes that need them. Defining them for
> "void *" is a poor idea in general IMO and a catastrophic one in case of wx
> (and Qt, as it seems).

While I don't mind using the system malloc by default, I'd like to consider other options here rather than a complete, hard-coded shutoff of fastMalloc. At the very least there should be a way to allow apps to turn on fastMalloc support in wxWebKit with a caveat somewhere. (e.g. have the wx port define USE_SYSTEM_MALLOC 1 in Platform.h if not already defined) Digsby has been shipping wxWebKit with it for over a year now with fastMalloc on and it seems to be working for them, so I'd like a way for them to enable it if they want without having to patch the codebase.

I think there has actually been an attempt at using fast malloc only for certain classes, but it seems the scope is still pretty limited. Considering that most ports seem to have it enabled for everything (including Safari/Mac and Safari/Win), I suspect that there's probably not a lot of incentive to finish it up...

[Vadim Zeitlin]

I don't really have anything to add, I think I gave all my arguments already but I'm still very, very surprised that you want to knowingly introduce an option to enable the possibility of fatal crashes undetectable during compile-time. I simply can't fathom why would anyone take such a risk, no performance gain can justify it IMO.

IMNSHO enabling fast malloc on per-class basis (as indicated by profiling results) is exactly the right thing to do and there should be no option to do it.

[Kevin Ollivier]

(In reply to comment #5)
> I don't really have anything to add, I think I gave all my arguments already
> but I'm still very, very surprised that you want to knowingly introduce an
> option to enable the possibility of fatal crashes undetectable during
> compile-time. I simply can't fathom why would anyone take such a risk, no
> performance gain can justify it IMO.

Well, the bottom line is that I'm not sure this is a very common problem in wxWebKit, if it exists anyplace else at all. Since wx does most things on the stack, even dealing with pointers in wx is not that common outside of wxWindow-derived objects, which typically do not transfer ownership. 

Again, I'm happy with the default being that fastMalloc is off, as playing it safe is of course the best default for people who are unfamiliar with all of this. But really, in the end, regardless of whether fastMalloc support is off or on, the only way to know if wxWebKit crashes or not (or does anything else it's not supposed to) is to runtime test it with your data. Or help me get DRT / LayoutTests completely going!

> IMNSHO enabling fast malloc on per-class basis (as indicated by profiling
> results) is exactly the right thing to do and there should be no option to do
> it.

Certainly, we both agree on that. Regardless, the reason there is a USE_SYSTEM_MALLOC define is precisely because that approach is not anywhere near fully implemented yet.

[Adam Barth]

Attachment 42256 [details] passed the style-queue

[Zoltan Horvath]

Heyhey,

(In reply to comment #0)
> So I think it's preferable to avoid this problem once and for all by just not
> using this allocator by default. As it was already done for Qt port (for the
> same reasons, I guess, although the comment is silent about it), I guess it
> shouldn't be that bad performance-wise but please correct me if I'm wrong.

At the case of QtWebKit, the problem is a little different (but have connections), you can read here (https://bugs.webkit.org/show_bug.cgi?id=31827#c2) and on the mailing list. 
The problem came forward only when TCmalloc was turned on and it had relations to the destructor calls.

I disabled customizing global operator new in QtWebKit, because at the same time I turned on TCmalloc. Turning off the customizing of the global new/delete operators is part of our FastAllocBase solution (about the solution you can read more at bug #20422). 

(In reply to comment #1)
> Created an attachment (id=42256) [details]
> Patch disabling global operators new and delete for wx port too

Ideally, when i will finish the changes of the class inheritance tree (there are less than ~15 classes left for the core part's of WebKit, and then will be only the port specific parts left), we'll disable customizing global operator new/delete, so you don't need the long comment in the patch. The customizing will be on class level and done fully through FastAllocBase class, with this technique we can achieve full allocation control and further checking.

> Sorry for not attaching the patch when creating the ticket (in fact I did but
> forgot to enter description and bugzilla decided to create the bug without
> attachment instead of failing to create it at all...).

This is not problem. :-) Btw, you should include the url of the bug into your changelog.

(In reply to comment #2)
> Could you provide some specific information about these crashes and how to
> trigger them? I'd like to take a closer look at this problem and see what our
> options are. After some googling I found that the QtWebKit devs did seem to
> have similar reasons for switching away, but they also mentioned that on
> Windows there's a significant performance increase (30%) when using the
> TCMalloc-based approach.
> 
> http://lists.macosforge.org/pipermail/webkit-dev/2008-September/004912.html
> 
> That number is big enough that it makes me wonder if it's not worth considering
> alternatives. Also, considering that FastMalloc predates the Windows
> implementation, or any other WebKit ports for that matter, I suspect there may
> be significant gains on Mac as well.

Effect of enabling TCmalloc in the Qt port:
- performance: http://webkit.sed.hu/node/23
- memory consumption: http://webkit.sed.hu/node/25

The QtWebKit-Windows port doesn't use the TCmalloc yet, because it is based on mingw compilation and it hasn't got pthreads. Anyway the normal windows port of WebKit ("Safari-port") uses TCmalloc, because it compiles in Cygwin. What does wxWebKit port do? What do you use to compile it?

(In reply to comment #3)
> overloaded global delete operator. But, of course, the m_data pointer was
> allocated inside wx itself and not with fast malloc overload of new operator so
> there is a mismatch and it crashes.

FAST_MALLOC_MATCH_VALIDATION macro can be the solution for this. If TCmalloc is enabled it will cause a good "debug-able" runtime error both on Linux and Windows.
 
> FWIW this particular problem is fixed now as I simply removed mb_str() in bug
> 30980 but others might remain and the worst is that you get a guaranteed crash
> which can't be detected at compile-time whenever you happen to delete anything
> allocated by wx in wxWebKit code, even if it happens in an implicit and
> roundabout way as above. So it would be extremely dangerous to not disable
> these operators for wx as plenty of stuff allocated by wx may be destroyed from
> user code.

Does the wxWebKit port use TCmalloc? If USE_SYSTEM_MALLOC is turned on, fastMalloc/fastDelete/new/delete uses the standard malloc/free implementation for it and it shouldn't cause errors.

> Concerning the performance, the real solution would be to define operators
> new/delete using fast malloc for the classes that need them. 

Eventually, we do this by changing the inheritance tree.

(In reply to comment #4)
> While I don't mind using the system malloc by default, I'd like to consider
> other options here rather than a complete, hard-coded shutoff of fastMalloc. At
> the very least there should be a way to allow apps to turn on fastMalloc
> support in wxWebKit with a caveat somewhere. (e.g. have the wx port define
> USE_SYSTEM_MALLOC 1 in Platform.h if not already defined) Digsby has been
> shipping wxWebKit with it for over a year now with fastMalloc on and it seems
> to be working for them, so I'd like a way for them to enable it if they want
> without having to patch the codebase.

If you want to turn on - if it not turned on yet (sorry, but I'm not familiar with the wx-port) - TCmalloc for the wx port, it is a right thing to turn off customizing global operator new/delete.  With not customized global operator new/delete it should work always.

> I think there has actually been an attempt at using fast malloc only for
> certain classes, but it seems the scope is still pretty limited. Considering
> that most ports seem to have it enabled for everything (including Safari/Mac
> and Safari/Win), I suspect that there's probably not a lot of incentive to
> finish it up...

The scope isn't limited, as I wrote there are < ~15 classes left for the core of WebKit (now, = not port related parts).

(In reply to comment #6)
> (In reply to comment #5)
> > I don't really have anything to add, I think I gave all my arguments already
> > but I'm still very, very surprised that you want to knowingly introduce an
> > option to enable the possibility of fatal crashes undetectable during
> > compile-time. I simply can't fathom why would anyone take such a risk, no
> > performance gain can justify it IMO.
> 
> Well, the bottom line is that I'm not sure this is a very common problem in
> wxWebKit, if it exists anyplace else at all. Since wx does most things on the
> stack, even dealing with pointers in wx is not that common outside of
> wxWindow-derived objects, which typically do not transfer ownership. 
> 
> Again, I'm happy with the default being that fastMalloc is off, as playing it
> safe is of course the best default for people who are unfamiliar with all of
> this. But really, in the end, regardless of whether fastMalloc support is off
> or on, the only way to know if wxWebKit crashes or not (or does anything else
> it's not supposed to) is to runtime test it with your data. Or help me get DRT
> / LayoutTests completely going!
> 
> > IMNSHO enabling fast malloc on per-class basis (as indicated by profiling
> > results) is exactly the right thing to do and there should be no option to do
> > it.
> 
> Certainly, we both agree on that. Regardless, the reason there is a
> USE_SYSTEM_MALLOC define is precisely because that approach is not anywhere
> near fully implemented yet.

After all :-)

If you want to turn on TCmalloc in secure way:
1. remove USE_SYSTEM_MALLOC = 1 macro or use USE_SYSTEM_MALLOC = 0 for wxWebKit
2.  remove customizing global operator new/delete as the patch contains.
(3. you can use mismatch checking by using FAST_MALLOC_MATCH_VALIDATION macro)

If you want to use system allocator just use USE_SYSTEM_MALLOC = 1 macro and global operator new/delete customizing shouldn't matter.

I prefer to use TCmalloc, because the main ports are use it also!

[Kevin Ollivier]

(In reply to comment #8)
> Heyhey,
> 
> (In reply to comment #0)
> > So I think it's preferable to avoid this problem once and for all by just not
> > using this allocator by default. As it was already done for Qt port (for the
> > same reasons, I guess, although the comment is silent about it), I guess it
> > shouldn't be that bad performance-wise but please correct me if I'm wrong.
> 
> At the case of QtWebKit, the problem is a little different (but have
> connections), you can read here
> (https://bugs.webkit.org/show_bug.cgi?id=31827#c2) and on the mailing list. 
> The problem came forward only when TCmalloc was turned on and it had relations
> to the destructor calls.
> 
> I disabled customizing global operator new in QtWebKit, because at the same
> time I turned on TCmalloc. Turning off the customizing of the global new/delete
> operators is part of our FastAllocBase solution (about the solution you can
> read more at bug #20422). 
> 
> (In reply to comment #1)
> > Created an attachment (id=42256) [details] [details]
> > Patch disabling global operators new and delete for wx port too
> 
> Ideally, when i will finish the changes of the class inheritance tree (there
> are less than ~15 classes left for the core part's of WebKit, and then will be
> only the port specific parts left), we'll disable customizing global operator
> new/delete, so you don't need the long comment in the patch. The customizing
> will be on class level and done fully through FastAllocBase class, with this
> technique we can achieve full allocation control and further checking.

Is there a (meta)bug for tracking the progress of this and seeing what is left to do? If there's less than 15 classes left, it would seem that it shouldn't be a great deal of work to finish up the changes.

> (In reply to comment #2)
[snip]
> The QtWebKit-Windows port doesn't use the TCmalloc yet, because it is based on
> mingw compilation and it hasn't got pthreads. Anyway the normal windows port of
> WebKit ("Safari-port") uses TCmalloc, because it compiles in Cygwin. What does
> wxWebKit port do? What do you use to compile it?

We use MSVC 8+ for compilation, and we are using TCmalloc, which is how this crash was found in the first place.

> (In reply to comment #3)
> > overloaded global delete operator. But, of course, the m_data pointer was
> > allocated inside wx itself and not with fast malloc overload of new operator so
> > there is a mismatch and it crashes.
> 
> FAST_MALLOC_MATCH_VALIDATION macro can be the solution for this. If TCmalloc is
> enabled it will cause a good "debug-able" runtime error both on Linux and
> Windows.

Thanks for the tip!

[snip]

> > Certainly, we both agree on that. Regardless, the reason there is a
> > USE_SYSTEM_MALLOC define is precisely because that approach is not anywhere
> > near fully implemented yet.
> 
> After all :-)
> 
> If you want to turn on TCmalloc in secure way:
> 1. remove USE_SYSTEM_MALLOC = 1 macro or use USE_SYSTEM_MALLOC = 0 for wxWebKit
> 2.  remove customizing global operator new/delete as the patch contains.
> (3. you can use mismatch checking by using FAST_MALLOC_MATCH_VALIDATION macro)
> 
> If you want to use system allocator just use USE_SYSTEM_MALLOC = 1 macro and
> global operator new/delete customizing shouldn't matter.
> 
> I prefer to use TCmalloc, because the main ports are use it also!

Yes, we'd like to use TCmalloc too, and if most classes are using FastAllocBase now then perhaps we wouldn't see much of a performance drop by turning off the global new/delete operator. That depends on what classes remain, though, and the frequency in which those objects are allocated. Any pointers to info on what's left to do for finishing up WebCore's usage of FastAllocBase whenever needed would be greatly appreciated!

[Kevin Ollivier]

@Zoltan

Update: I've tried using ENABLE_FAST_MALLOC_MATCH_VALIDATION and while I was able to compile the wx port with it, it crashes in common code. Worse, this happens even when I define USE_SYSTEM_MALLOC and set it to 1, so it seems this mismatch is not related to problems with global new / delete overrides. (I've pasted the stack trace below.) I tried then to compile the Safari/Mac port to confirm it was a common problem, but Safari/Mac doesn't compile with ENABLE_FAST_MALLOC_MATCH_VALIDATION set because FastMalloc.cpp (on line 3590 in the latest revision as of this time) references an undefined variable pByte. This problem seemed to be there even when the code was initially committed (http://trac.webkit.org/changeset/42344/trunk/JavaScriptCore/wtf/FastMalloc.cpp) So I'm not sure if anyone has ever used the fast malloc checking on Safari/Mac. :( I'm also not sure why that code isn't being compiled for wx, though there's quite many #ifdefs in there, so I suppose that code is conditional upon some define.

Are any of the ports using this regularly to check for malloc mismatches? Do you know?

Finally, here is the trace: 

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xbbadbeef
0x0003eb8b in WTF::Internal::fastMallocMatchFailed () at ../wtf/FastMalloc.cpp:168
168	    CRASH();
(gdb) bt
#0  0x0003eb8b in WTF::Internal::fastMallocMatchFailed () at ../wtf/FastMalloc.cpp:168
#1  0x0003df94 in WTF::fastMallocMatchValidateFree (p=0x18e21f78, allocType=WTF::Internal::AllocTypeClassNew) at FastMalloc.h:164
#2  0x0003e049 in WTF::FastAllocBase::operator delete (p=0x18e21f78) at FastAllocBase.h:103
#3  0x003745a5 in WebCore::CSSSelectorList::deleteSelectors (this=0x18e20d3c) at ../css/CSSSelectorList.cpp:79
#4  0x00374616 in WebCore::CSSSelectorList::~CSSSelectorList (this=0x18e20d3c) at ../css/CSSSelectorList.cpp:34
#5  0x00375a16 in WebCore::CSSStyleRule::~CSSStyleRule (this=0x18e20d28) at ../css/CSSStyleRule.cpp:39
#6  0x003a7e35 in WTF::RefCounted<WebCore::StyleBase>::deref (this=0x18e20d2c) at RefCounted.h:109
#7  0x00358cac in WTF::RefPtr<WebCore::StyleBase>::~RefPtr (this=0x18e221fc) at RefPtr.h:53
#8  0x00358cc7 in WTF::VectorDestructor<true, WTF::RefPtr<WebCore::StyleBase> >::destruct (begin=0x18e221f8, end=0x18e2220c) at Vector.h:88
#9  0x00358cf0 in WTF::VectorTypeOperations<WTF::RefPtr<WebCore::StyleBase> >::destruct (begin=0x18e221f8, end=0x18e2220c) at Vector.h:243
#10 0x00358d75 in WTF::Vector<WTF::RefPtr<WebCore::StyleBase>, 0ul>::shrink (this=0x18e21b18, size=0) at Vector.h:768
#11 0x00358da7 in WTF::Vector<WTF::RefPtr<WebCore::StyleBase>, 0ul>::~Vector (this=0x18e21b18) at Vector.h:491
#12 0x003d095a in WebCore::StyleList::~StyleList (this=0x18e21b08) at StyleList.h:33
#13 0x003d04d1 in WebCore::StyleSheet::~StyleSheet (this=0x18e21b08) at ../css/StyleSheet.cpp:56
#14 0x003bfc8d in WebCore::CSSStyleSheet::~CSSStyleSheet (this=0x18e21b08) at ../css/CSSStyleSheet.cpp:74
#15 0x0039c31b in loadFullDefaultStyle () at ../css/CSSStyleSelector.cpp:523
#16 0x0039d16f in WebCore::CSSStyleSelector::styleForElement (this=0x1a807608, e=0x18e83ba8, defaultParent=0x0, allowSharing=true, resolveForRootDefault=false) at ../css/CSSStyleSelector.cpp:1129
#17 0x00438bee in WebCore::Node::styleForRenderer (this=0x18e83ba8) at ../dom/Node.cpp:1409
#18 0x0043ca62 in WebCore::Node::createRendererIfNeeded (this=0x18e83ba8) at ../dom/Node.cpp:1386
#19 0x00422c07 in WebCore::Element::attach (this=0x18e83ba8) at ../dom/Element.cpp:735
#20 0x0058a4c9 in WebCore::HTMLParser::insertNode (this=0x18e71bd8, n=0x18e83ba8, flat=false) at ../html/HTMLParser.cpp:375
#21 0x0058b21c in WebCore::HTMLParser::parseToken (this=0x18e71bd8, t=0x1a80c024) at ../html/HTMLParser.cpp:274
#22 0x005a2177 in WebCore::HTMLTokenizer::processToken (this=0x1a80c008) at ../html/HTMLTokenizer.cpp:1934
#23 0x005a8b25 in WebCore::HTMLTokenizer::parseTag (this=0x1a80c008, src=@0x1a80c9b0, state={static EntityShift = 4, m_bits = 0}) at ../html/HTMLTokenizer.cpp:1506
#24 0x005a991a in WebCore::HTMLTokenizer::write (this=0x1a80c008, str=@0xbfffd8f0, appendData=true) at ../html/HTMLTokenizer.cpp:1757
#25 0x00647930 in WebCore::FrameLoader::write (this=0x1a80a034, str=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., len=1373, flush=false) at ../loader/FrameLoader.cpp:913
#26 0x00647ac3 in WebCore::FrameLoader::addData (this=0x1a80a034, bytes=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373) at ../loader/FrameLoader.cpp:1466
#27 0x0005f3de in WebCore::FrameLoaderClientWx::committedLoad (this=0x18e17ae0, loader=0x6808e08, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373) at ../WebKitSupport/FrameLoaderClientWx.cpp:572
#28 0x00641d6e in WebCore::FrameLoader::committedLoad (this=0x1a80a034, loader=0x6808e08, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373) at ../loader/FrameLoader.cpp:3209
#29 0x00629fba in WebCore::DocumentLoader::commitLoad (this=0x6808e08, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373) at ../loader/DocumentLoader.cpp:342
#30 0x0062a038 in WebCore::DocumentLoader::receivedData (this=0x6808e08, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373) at ../loader/DocumentLoader.cpp:354
#31 0x0064113b in WebCore::FrameLoader::receivedData (this=0x1a80a034, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373) at ../loader/FrameLoader.cpp:2061
#32 0x00669314 in WebCore::MainResourceLoader::addData (this=0x6804008, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373, allAtOnce=false) at ../loader/MainResourceLoader.cpp:143
#33 0x00675697 in WebCore::ResourceLoader::didReceiveData (this=0x6804008, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373, lengthReceived=0, allAtOnce=false) at ../loader/ResourceLoader.cpp:248
#34 0x0066971a in WebCore::MainResourceLoader::didReceiveData (this=0x6804008, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373, lengthReceived=0, allAtOnce=false) at ../loader/MainResourceLoader.cpp:374
#35 0x00675328 in WebCore::ResourceLoader::didReceiveData (this=0x6804008, data=0x1a84c200 "<!doctype html><html><head><meta http-equiv=\"content-type\" content=\"text/html; charset=UTF-8\"><title>Google</title><script>window.google={kEI:\"C8U2S9T0CYqIowTC8cSSBw\",kEXPI:\"17259,23129\",kCSI:{e:\"1725"..., length=1373, lengthReceived=0) at ../loader/ResourceLoader.cpp:398
#36 0x007bec20 in writeCallback (ptr=0x1a84c200, size=1, nmemb=1373, data=0x18c0f068) at ../platform/network/curl/ResourceHandleManager.cpp:198
#37 0x0446787a in Curl_client_write ()
#38 0x04482146 in inflate_stream ()
#39 0x044825ef in Curl_unencode_gzip_write ()
#40 0x0447ca3b in Curl_readwrite ()
#41 0x04481867 in multi_runsingle ()
#42 0x04481c1f in curl_multi_perform ()
#43 0x007c1469 in WebCore::ResourceHandleManager::downloadTimerCallback (this=0x5b13f30, timer=0x5b13f30) at ../platform/network/curl/ResourceHandleManager.cpp:352
#44 0x007c30e9 in WebCore::Timer<WebCore::ResourceHandleManager>::fired (this=0x5b13f30) at Timer.h:98
#45 0x00759468 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x18e15b98) at ../platform/ThreadTimers.cpp:112
#46 0x00759537 in WebCore::ThreadTimers::sharedTimerFired () at ../platform/ThreadTimers.cpp:90
#47 0x007f4563 in WebCore::WebKitTimer::Notify (this=0x5b20120) at ../platform/wx/SharedTimerWx.cpp:61
#48 0x03adf5f8 in wxTimerImpl::Notify (this=0x5b20190) at private/timer.h:48
#49 0x03adeed5 in wxProcessTimer (data=0x5b20190) at timer.cpp:39
#50 0x91cf8edb in __CFRunLoopRun ()
#51 0x91cf6864 in CFRunLoopRunSpecific ()
#52 0x91cf6691 in CFRunLoopRunInMode ()
#53 0x95b32f0c in RunCurrentEventLoopInMode ()
#54 0x95b32cc3 in ReceiveNextEventCommon ()
#55 0x95b32b48 in BlockUntilNextEventMatchingListInMode ()
#56 0x90975ac5 in _DPSNextEvent ()
#57 0x90975306 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#58 0x03b5c0e7 in wxGUIEventLoop::Dispatch (this=0x18e0b110) at evtloop.mm:122
#59 0x0420abf1 in wxEventLoopManual::ProcessEvents (this=0x18e0b110) at evtloopcmn.cpp:109
#60 0x0420acea in wxEventLoopManual::Run (this=0x18e0b110) at evtloopcmn.cpp:159
#61 0x041dcf61 in wxAppConsoleBase::MainLoop (this=0x5b00110) at appbase.cpp:318
#62 0x041dc51a in wxAppConsoleBase::OnRun (this=0x5b00110) at appbase.cpp:259
#63 0x03b7ac14 in wxAppBase::OnRun (this=0x5b00110) at appcmn.cpp:285
#64 0x03b05d51 in wxApp::OnRun (this=0x5b00110) at app.cpp:874
#65 0x0423d5ce in wxEntry () at init.cpp:462
#66 0x0423d9a9 in wxEntry (argc=@0xbffff450, argv=0xbffff470) at init.cpp:474
#67 0x00002798 in main (argc=1, argv=0xbffff470) at ../browser.cpp:45

[Zoltan Horvath]

I'm on a holiday now and be without Internet in the following days. I'll check this in January. Btw, I used it in Qt-port when we landed the patch.

[Benbuck Nason]

Sorry if this is going off topic. When I set ENABLE_FAST_MALLOC_MATCH_VALIDATION to 1 I got the same result as Kevin in comment #10. To fix the mismatch I had to make two changes:

In CSSSelectorList::deleteSelectors():

    if (done)
        fastDelete(s); // was using delete


And in CSSSelector::~CSSSelector():

    if (m_hasRareData)
        delete m_data.m_rareData;
    else
        fastDelete(m_data.m_tagHistory); // was using delete

Should these changes be added to the trunk, or is this configuration option not supported?

[Zoltan Horvath]

I corrected the pByte compilation problem. It was an older varible name, we left it somehow when we added the FastMalloc validation to the trunk. Now it still doesn't compile on MAC due to other problems. I'm investigating it.

I got the same error with CSSSelectorList the mismatch should be corrected somehow, feel free to file patch if you have one already.

At this week I'm trying to put more effort on this topic.

[Zoltan Horvath]

I tried out your solution. I reopened the bug of the mismatch (bug #22834).
We need to check that the comment in CSSSelectorList.cpp is still valid. Benbuck please file your patch there.

[Zoltan Horvath]

@Kevin: now we have << 5 classes in the core WebCore (in the platform independent code) which is instantiated by new but not inherited from FastAllocBase, so you can disable global overriding new/delete for wx port. I think it won't cause significant performance lose when using TCmalloc.

[Eric Seidel (no email)]

ping?  What's the status of this patch? Should it still be up for review?

[Zoltan Horvath]

Comment on attachment 42256 [details]
Patch disabling global operators new and delete for wx port too

I mark this patch r- just because we don't need to add these comments into FastMalloc.h.

Darin summarization about new/delete operators:
https://lists.webkit.org/pipermail/webkit-dev/2010-January/011276.html

As I said, almost all core classes are inherited from FastAllocBase, so disabling customization of operator new/delete is safe and should not have performance effect.

[Kevin Ollivier]

Created attachment 47495 [details]
Make an ENABLE switch for using fastmalloc new so any port can easily customize behavior

Given all the comments so far, it seems like the global new/delete override offers much less value than it used to, and that whether it should be enabled or not is not just a question for the wx port these days. I've attached a patch to make it configurable via Platform.h, as I think it's cleaner and easier than having each port edit FastMalloc.h, and I'm turning it off for now in wx.

[Eric Seidel (no email)]

I'm unsure what the effect of turning off the global new/delete overrides would be.  Since many (but not all!) of the classes which depend on fastMalloc new/delete use FastMallocBase or NonCopyable you'd end up with many (but not all) WebCore classes using fastMalloc and some using the system malloc.  I don't think that would cause any crashes, but I'm not enough of an expert to know.

Another solution would just be to turn on the USE_SYSTEM_MALLOC flag in FastMalloc for Wx, no?

Darin Adler is really a better reviewer for this than I am.

[Kevin Ollivier]

(In reply to comment #19)
> I'm unsure what the effect of turning off the global new/delete overrides would
> be.  Since many (but not all!) of the classes which depend on fastMalloc
> new/delete use FastMallocBase or NonCopyable you'd end up with many (but not
> all) WebCore classes using fastMalloc and some using the system malloc.  I
> don't think that would cause any crashes, but I'm not enough of an expert to
> know.

I won't say it can't crash, but any crashes it would introduce would be due to bugs in WebCore code, because it could only happen when mismatching explicit calls to the fast* functions with new / delete calls, which we should not be doing.

Note also that I don't change the setting for any port other than wx. I just make it possible to change the setting in Platform.h because if more ports do decide to turn it off, we'll end up with a pretty large and unruly #if statement in FastMalloc.h. From the comments in this thread, I anticipated that more ports may start turning it off soon, but if this patch isn't wanted I can certainly remove it.

> Another solution would just be to turn on the USE_SYSTEM_MALLOC flag in
> FastMalloc for Wx, no?

Yes, we could do that too, though we also lose the benefits of using TCMalloc in the process.

[Eric Seidel (no email)]

Comment on attachment 47495 [details]
Make an ENABLE switch for using fastmalloc new so any port can easily customize behavior

OK.

[Zoltan Horvath]

Comment on attachment 47495 [details]
Make an ENABLE switch for using fastmalloc new so any port can easily customize behavior

Clearing flags on attachment: 47495

Committed r55770: <http://trac.webkit.org/changeset/55770>

[Zoltan Horvath]

All reviewed patches have been landed.  Closing bug.