30977 2009-10-31 07:12:09 -0700 [Wx] Don't use global operators new/delete in wx port 2010-03-10 01:03:36 -0800 1 1 1 Unclassified WebKit WebKit wx 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 vz-webkit kevino abarth bnason eric kevino kevinwatters laszlo.gombos mjs ppedriana zoltan oldest_to_newest 159558 0 vz-webkit 2009-10-31 07:12:09 -0700 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! 159559 1 42256 vz-webkit 2009-10-31 07:13:42 -0700 Created attachment 42256 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...). 161021 2 kevino 2009-11-05 16:55:37 -0800 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. 161022 3 vz-webkit 2009-11-05 17:02:10 -0800 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). 161828 4 kevino 2009-11-09 16:42:49 -0800 (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... 161869 5 vz-webkit 2009-11-09 17:47:18 -0800 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. 162054 6 kevino 2009-11-10 08:14:47 -0800 (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. 167328 7 abarth 2009-11-30 12:22:29 -0800 Attachment 42256 passed the style-queue 167406 8 zoltan 2009-11-30 13:35:05 -0800 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! 167708 9 kevino 2009-12-01 12:56:28 -0800 (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! 174976 10 kevino 2009-12-26 18:52:20 -0800 @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 174988 11 zoltan 2009-12-26 22:50:13 -0800 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. 179529 12 bnason 2010-01-11 13:58:11 -0800 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? 179543 13 zoltan 2010-01-11 14:26:32 -0800 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. 179812 14 zoltan 2010-01-12 06:21:38 -0800 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. 179815 15 zoltan 2010-01-12 06:29:17 -0800 @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. 184719 16 eric 2010-01-26 14:05:23 -0800 ping? What's the status of this patch? Should it still be up for review? 184747 17 42256 zoltan 2010-01-26 14:36:42 -0800 Comment on attachment 42256 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. 184944 18 47495 kevino 2010-01-26 22:13:37 -0800 Created attachment 47495 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. 187985 19 eric 2010-02-04 15:56:55 -0800 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. 188065 20 kevino 2010-02-04 21:34:34 -0800 (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. 196600 21 47495 eric 2010-03-05 13:03:31 -0800 Comment on attachment 47495 Make an ENABLE switch for using fastmalloc new so any port can easily customize behavior OK. 197923 22 47495 zoltan 2010-03-10 01:03:25 -0800 Comment on attachment 47495 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> 197924 23 zoltan 2010-03-10 01:03:36 -0800 All reviewed patches have been landed. Closing bug. 42256 2009-10-31 07:13:42 -0700 2010-01-26 22:13:37 -0800 Patch disabling global operators new and delete for wx port too no-fast-alloc.diff text/plain 2025 vz-webkit Y29tbWl0IGFlZWMzNmUxOThjMjZlOTgzYjk2OWU0ZjZhOTUyOTExY2IxMGM0NzcKQXV0aG9yOiBW YWRpbSBaZWl0bGluIDx2YWRpbUB3eHdpZGdldHMub3JnPgpEYXRlOiAgIFNhdCBPY3QgMzEgMTQ6 NTY6MjUgMjAwOSArMDEwMAoKICAgIERpc2FibGUgZ2xvYmFsIG9wZXJhdG9ycyBuZXcgYW5kIGRl bGV0ZSBmb3Igd3ggcG9ydC4KICAgIAogICAgVXNpbmcgb3ZlcmxvYWRlZCBvcGVyYXRvcnMgbmV3 IGFuZCBkZWxldGUgcmVzdWx0ZWQgaW4gY3Jhc2hlcyB3aGVuIGZyZWVpbmcKICAgIG9iamVjdHMg YWxsb2NhdGVkIGluIHd4IGZyb20gV2ViS2l0IGNvZGUgZHVlIHRvIG1pc21hdGNoIG9mIGZ1bmN0 aW9ucyB1c2VkIGZvcgogICAgYWxsb2NhdGlvbiBhbmQgZGVhbGxvY2F0aW9uLgoKZGlmZiAtLWdp dCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwpp bmRleCA2ZDQzOWJlLi40NWFiM2Y1IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VM b2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTEgQEAKKzIwMDkt MTAtMzEgIFZhZGltIFplaXRsaW4gIDx2YWRpbUB3eHdpZGdldHMub3JnPgorCisgICAgICAgIFt3 eF0gRGlzYWJsZSBnbG9iYWwgbmV3L2RlbGV0ZSBvcGVyYXRvcnMgZm9yIHd4IHBvcnQgYXMgdGhp cyByZXN1bHRlZAorICAgICAgICBpbiBjcmFzaGVzIGR1ZSB0byBhbGxvY2F0aW9uIG1pc21hdGNo ZXMgd2hlbiBkZWxldGluZyBwb2ludGVycworICAgICAgICBhbGxvY2F0ZWQgaW5zaWRlIHd4IGl0 c2VsZiAoYW5kIGhlbmNlIG5vdCB1c2luZyBmYXN0TWFsbG9jKQorCisgICAgICAgICogd3RmL0Zh c3RNYWxsb2MuaDoKKwogMjAwOS0xMC0xNiAgR2VvZmZyZXkgR2FyZW4gIDxnZ2FyZW5AYXBwbGUu Y29tPgogCiAgICAgICAgIEJ1aWxkIGZpeDogYXBwYXJlbnRseSB3ZSBzaG91bGRuJ3QgZXhwb3J0 IHRob3NlIHN5bWJvbHM/CmRpZmYgLS1naXQgYS9KYXZhU2NyaXB0Q29yZS93dGYvRmFzdE1hbGxv Yy5oIGIvSmF2YVNjcmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuaAppbmRleCBjYTA5NjFjLi5iNmZj NDkxIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29yZS93dGYvRmFzdE1hbGxvYy5oCisrKyBiL0ph dmFTY3JpcHRDb3JlL3d0Zi9GYXN0TWFsbG9jLmgKQEAgLTIxMyw4ICsyMTMsMTIgQEAgdXNpbmcg V1RGOjpmYXN0TWFsbG9jQWxsb3c7CiAvLyBkZWJ1Zy1vbmx5IGNvZGUgdG8gbWFrZSBzdXJlIHdl IGRvbid0IHVzZSB0aGUgc3lzdGVtIG1hbGxvYyB2aWEgdGhlIGRlZmF1bHQgb3BlcmF0b3IKIC8v IG5ldyBieSBhY2NpZGVudC4KIAotLy8gV2UgbXVzbid0IGN1c3RvbWl6ZSB0aGUgZ2xvYmFsIG9w ZXJhdG9yIG5ldyBhbmQgZGVsZXRlIGZvciB0aGUgUXQgcG9ydC4KLSNpZiAhUExBVEZPUk0oUVQp CisvLyBXZSBtdXN0bid0IGN1c3RvbWl6ZSB0aGUgZ2xvYmFsIG9wZXJhdG9yIG5ldyBhbmQgZGVs ZXRlIGZvciB0aGUgUXQgcG9ydCBub3IKKy8vIGZvciB3eCBvbmUgKGZvciB0aGUgbGF0dGVyLCB0 aGlzIHdvdWxkIHJlc3VsdCBpbiBjcmFzaGVzIGF0IGxlYXN0IHdoZW4gY3JlYXRpbmcKKy8vIHd4 U2NvcGVkQ2hhclR5cGVCdWZmZXIgb2JqZWN0cywgZS5nLiBiZWNhdXNlIG9mIGNhbGxzIHRvIHd4 U3RyaW5nOjp1dGY4X3N0cigpLCAKKy8vIGJlY2F1c2UgaXRzIGR0b3IgdXNlcyBkZWxldGUgZm9y IGEgcG9pbnRlciBhbGxvY2F0ZWQgaW5zaWRlIHd4IGl0c2VsZiBhbmQgc28KKy8vIG5vdCBhbGxv Y2F0ZWQgdXNpbmcgZmFzdE1hbGxvYygpKQorI2lmICEoUExBVEZPUk0oUVQpIHx8IFBMQVRGT1JN KFdYKSkKIAogV1RGX1BSSVZBVEVfSU5MSU5FIHZvaWQqIG9wZXJhdG9yIG5ldyhzaXplX3Qgc2l6 ZSkgeyByZXR1cm4gZmFzdE1hbGxvYyhzaXplKTsgfQogV1RGX1BSSVZBVEVfSU5MSU5FIHZvaWQq IG9wZXJhdG9yIG5ldyhzaXplX3Qgc2l6ZSwgY29uc3Qgc3RkOjpub3Rocm93X3QmKSB0aHJvdygp IHsgcmV0dXJuIGZhc3RNYWxsb2Moc2l6ZSk7IH0K 47495 2010-01-26 22:13:37 -0800 2010-03-10 01:03:25 -0800 Make an ENABLE switch for using fastmalloc new so any port can easily customize behavior global_fastmalloc_new_switch.patch text/plain 2097 kevino SW5kZXg6IEphdmFTY3JpcHRDb3JlL3d0Zi9GYXN0TWFsbG9jLmgKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gSmF2 YVNjcmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuaAkocmV2aXNpb24gNTM4OTQpCisrKyBKYXZhU2Ny aXB0Q29yZS93dGYvRmFzdE1hbGxvYy5oCSh3b3JraW5nIGNvcHkpCkBAIC0yMTUsOCArMjE1LDcg QEAKIC8vIGRlYnVnLW9ubHkgY29kZSB0byBtYWtlIHN1cmUgd2UgZG9uJ3QgdXNlIHRoZSBzeXN0 ZW0gbWFsbG9jIHZpYSB0aGUgZGVmYXVsdCBvcGVyYXRvcgogLy8gbmV3IGJ5IGFjY2lkZW50Lgog Ci0vLyBXZSBtdXNuJ3QgY3VzdG9taXplIHRoZSBnbG9iYWwgb3BlcmF0b3IgbmV3IGFuZCBkZWxl dGUgZm9yIHRoZSBRdCBwb3J0LgotI2lmICFQTEFURk9STShRVCkKKyNpZiBFTkFCTEUoR0xPQkFM X0ZBU1RNQUxMT0NfTkVXKQogCiAjaWYgQ09NUElMRVIoTVNWQykKICNwcmFnbWEgd2FybmluZyhw dXNoKQpJbmRleDogSmF2YVNjcmlwdENvcmUvd3RmL1BsYXRmb3JtLmgKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g SmF2YVNjcmlwdENvcmUvd3RmL1BsYXRmb3JtLmgJKHJldmlzaW9uIDUzODk0KQorKysgSmF2YVNj cmlwdENvcmUvd3RmL1BsYXRmb3JtLmgJKHdvcmtpbmcgY29weSkKQEAgLTU5OCw2ICs1OTgsNyBA QAogCiAjaWYgUExBVEZPUk0oV1gpCiAjZGVmaW5lIEVOQUJMRV9BU1NFTUJMRVIgMQorI2RlZmlu ZSBFTkFCTEVfR0xPQkFMX0ZBU1RNQUxMT0NfTkVXIDAKICNpZiBPUyhEQVJXSU4pCiAjZGVmaW5l IFdURl9QTEFURk9STV9DRiAxCiAjZW5kaWYKQEAgLTcyNSw2ICs3MjYsMTEgQEAKIAogLyogRU5B QkxFIG1hY3JvIGRlZmF1bHRzICovCiAKKyNpZiBQTEFURk9STShRVCkKKy8vIFdlIG11c24ndCBj dXN0b21pemUgdGhlIGdsb2JhbCBvcGVyYXRvciBuZXcgYW5kIGRlbGV0ZSBmb3IgdGhlIFF0IHBv cnQuCisjZGVmaW5lIEVOQUJMRV9HTE9CQUxfRkFTVE1BTExPQ19ORVcgMAorI2VuZGlmCisKIC8q IGZhc3RNYWxsb2MgbWF0Y2ggdmFsaWRhdGlvbiBhbGxvd3MgZm9yIHJ1bnRpbWUgdmVyaWZpY2F0 aW9uIHRoYXQKICAgIG5ldyBpcyBtYXRjaGVkIGJ5IGRlbGV0ZSwgZmFzdE1hbGxvYyBpcyBtYXRj aGVkIGJ5IGZhc3RGcmVlLCBldGMuICovCiAjaWYgIWRlZmluZWQoRU5BQkxFX0ZBU1RfTUFMTE9D X01BVENIX1ZBTElEQVRJT04pCkBAIC03ODMsNiArNzg5LDEwIEBACiAjZGVmaW5lIEVOQUJMRV9P UENPREVfU1RBVFMgMAogI2VuZGlmCiAKKyNpZiAhZGVmaW5lZChFTkFCTEVfR0xPQkFMX0ZBU1RN QUxMT0NfTkVXKQorI2RlZmluZSBFTkFCTEVfR0xPQkFMX0ZBU1RNQUxMT0NfTkVXIDEKKyNlbmRp ZgorCiAjZGVmaW5lIEVOQUJMRV9TQU1QTElOR19DT1VOVEVSUyAwCiAjZGVmaW5lIEVOQUJMRV9T QU1QTElOR19GTEFHUyAwCiAjZGVmaW5lIEVOQUJMRV9PUENPREVfU0FNUExJTkcgMApJbmRleDog SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFTY3JpcHRDb3JlL0No YW5nZUxvZwkocmV2aXNpb24gNTM4OTgpCisrKyBKYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cJKHdv cmtpbmcgY29weSkKQEAgLTEsMyArMSwxMyBAQAorMjAxMC0wMS0yNiAgS2V2aW4gT2xsaXZpZXIg IDxrZXZpbm9AdGhlb2xsaXZpZXJzLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBNYWtlIGdsb2JhbCBuZXcvZGVsZXRlIG9wZXJhdG9ycyBjb25m aWd1cmFibGUgZm9yIGFsbCBwb3J0cyBhbmQgZGlzYWJsZSBpdAorICAgICAgICBmb3IgdGhlIHd4 IHBvcnQgZm9yIG5vdy4KKworICAgICAgICAqIHd0Zi9GYXN0TWFsbG9jLmg6CisgICAgICAgICog d3RmL1BsYXRmb3JtLmg6CisKIDIwMTAtMDEtMjYgIEx5b24gQ2hlbiAgPGxpYWNoZW5AcmltLmNv bT4KIAogICAgICAgICBSZXZpZXdlZCBieSBNYWNpZWogU3RhY2hvd2lhay4K