Bug 41177

Summary: References for movable objects
Product: WebKit Reporter: Nathan Lawrence <nlawrence>
Component: JavaScriptCoreAssignee: Nathan Lawrence <nlawrence>
Status: REOPENED ---    
Severity: Normal CC: abarth, abecsi, commit-queue, eric, ggaren, gustavo, kent.hansen, nlawrence, ossy, slewis, webkit.review.bot, xan.lopez, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 43496, 43619    
Bug Blocks:    
Attachments:
Description Flags
Patch
ggaren: review-
Patch (now with code!)
none
Patch
ggaren: review-
Patch
none
Patch (no, really it should compile this time...)
none
Patch
darin: review-
Patch
none
Patch
darin: review-
Patch none

Description Nathan Lawrence 2010-06-24 14:25:22 PDT
Created attachment 59702 [details]
Patch

If MarkStack::append takes a reference, we can update most of the pointers from a single location, making movable objects simpler.
Comment 1 Mark Rowe (bdash) 2010-06-24 14:34:20 PDT
Is there more to this change than the ChangeLog entry?
Comment 2 Geoffrey Garen 2010-06-24 15:54:35 PDT
Comment on attachment 59702 [details]
Patch

More patch please :).
Comment 3 Nathan Lawrence 2010-06-28 11:39:41 PDT
Created attachment 59914 [details]
Patch (now with code!)
Comment 4 Nathan Lawrence 2010-06-28 11:44:11 PDT
Created attachment 59916 [details]
Patch
Comment 5 WebKit Review Bot 2010-06-28 11:44:28 PDT
Attachment 59914 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/Collector.cpp:762:  Declaration has space between type name and * in JSCell *cell  [whitespace/declaration] [3]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 WebKit Review Bot 2010-06-28 11:45:42 PDT
Attachment 59916 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/Collector.cpp:762:  Declaration has space between type name and * in JSCell *cell  [whitespace/declaration] [3]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Early Warning System Bot 2010-06-28 11:52:37 PDT
Attachment 59914 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3304932
Comment 8 Early Warning System Bot 2010-06-28 12:15:17 PDT
Attachment 59916 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3324983
Comment 9 WebKit Review Bot 2010-06-28 12:25:48 PDT
Attachment 59914 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3292967
Comment 10 Geoffrey Garen 2010-06-28 12:49:43 PDT
Comment on attachment 59916 [details]
Patch

Angry builders :(.
Comment 11 Nathan Lawrence 2010-06-28 17:08:58 PDT
Created attachment 59961 [details]
Patch

Should fix problems with 32 bit version.
Comment 12 Early Warning System Bot 2010-06-28 17:33:20 PDT
Attachment 59961 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3283905
Comment 13 WebKit Review Bot 2010-06-28 18:25:43 PDT
Attachment 59961 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3361009
Comment 14 Nathan Lawrence 2010-06-28 20:07:15 PDT
Created attachment 59979 [details]
Patch (no, really it should compile this time...)
Comment 15 Nathan Lawrence 2010-07-29 09:30:07 PDT
Created attachment 62960 [details]
Patch

Fixes how we mark NativeErrorConstructor
Comment 16 Kent Hansen 2010-07-29 10:49:10 PDT
What's the further plan with this? Compacting GC?
Comment 17 Darin Adler 2010-07-29 10:51:20 PDT
Comment on attachment 62960 [details]
Patch

> +        * bytecode/CodeBlock.cpp:
> +        (JSC::CodeBlock::markAggregate):
> +        * runtime/Collector.cpp:
> +        (JSC::Heap::markConservatively):
> +        * runtime/JSCell.h:
> +        (JSC::JSValue::asCell):
> +        (JSC::MarkStack::append):
> +        (JSC::MarkStack::appendInternal):
> +        * runtime/JSGlobalObject.cpp:
> +        (JSC::markIfNeeded):
> +        * runtime/JSONObject.cpp:
> +        (JSC::Stringifier::Holder::object):
> +        * runtime/JSObject.h:
> +        (JSC::JSObject::prototype):
> +        * runtime/JSStaticScopeObject.cpp:
> +        (JSC::JSStaticScopeObject::markChildren):
> +        * runtime/JSValue.h:
> +        (JSC::JSValue::):
> +        (JSC::JSValue::JSValue):
> +        (JSC::JSValue::asCell):
> +        * runtime/MarkStack.h:
> +        * runtime/NativeErrorConstructor.cpp:
> +        (JSC::NativeErrorConstructor::createStructure):
> +        (JSC::NativeErrorConstructor::markChildren):
> +        * runtime/NativeErrorConstructor.h:
> +        * runtime/Structure.h:
> +        (JSC::Structure::storedPrototype):

If you're not going to write function by function comments, which is OK, I suggest you remove stray bogus function names that prepare-ChangeLog puts in. For example "(JSC::JSValue::)". I personally prefer to include function by function comments. A useful way to think through your patch one extra time while writing a brief explanation of each change.

> -                markStack.append(reinterpret_cast<JSCell*>(xAsBits));
> +                JSCell* cell = reinterpret_cast<JSCell*>(xAsBits);
> +                markStack.append(cell);

It's unclear why this is OK. Normally we need to call append on references in place, not temporary copies on the stack. I think a comment here is needed to explain why it's OK to do it this way here.

> +        friend class CollectorHeap;

Adding friends is OK, but always unfortunate. If there is a way to avoid it, that would be better.

> +        /* References in C++ are not covariant, so we fudge around it with
> +         * this nonsense.
> +         */

A couple thoughts about this comment:

    1) We use C-style "//" comments rather than "/*" comments in almost all cases. You should here.

    2) While it's fine to use an informal tone "fudge around it with this nonsense" is probably too vague for some readers. If you say something clearer then you have a chance to explain why this code is a good idea. For example, here is something helpful, but probably a bit too long: "That rule prevents writing a pointer of a base class type into a more-specifically typed pointer. But here, the only changes we will make would be to update the pointer to a moved copy of the same object, so there is no danger of changing the type. We use a typecast to bypass the normal rule." Calling the idiom "nonsense" will create unnecessary fear, uncertainty, and doubt for in future readers of the code.

> +    ALWAYS_INLINE void MarkStack::append(Register& reg)

Please don't use a non-word abbreviation when there's a word available. I guess you're avoiding "register" because it's a C++ keyword? Is there another word you can use here?

> -static inline void markIfNeeded(MarkStack& markStack, JSValue v)
> +template<typename T>
> +static void markIfNeeded(MarkStack& markStack, T*& v)

I think this would read better on a single line rather than two lines.

> -static inline void markIfNeeded(MarkStack& markStack, const RefPtr<Structure>& s)
> +static void markIfNeeded(MarkStack& markStack, RefPtr<Structure>& s)

Why did you remove the inline keyword here? Per-function comments in the change log are a great place to explain decisions like this one.

> +        template<typename T>
> +        ALWAYS_INLINE void append(T*& cell);
> +        ALWAYS_INLINE void append(JSValue&);
> +        ALWAYS_INLINE void append(Register&);

Here the function template declaration would be much easier to read if it was on one line rather than two!

> +void NativeErrorConstructor::markChildren(MarkStack& markStack)
> +{
> +    InternalFunction::markChildren(markStack);
> +    // HACKY_FIX:
> +    // The prototype pointer here needs to be updated
> +    //
> +    // Not sure if there's a better solution
> +    markStack.append(m_errorStructure->storedPrototype());
> +}

I don't think "HACKY_FIX" here is appropriate. If you think there's something wrong here, you need to say what. This code looks just fine to me, so there must be something specific about it that seems wrong to you. You should say what it is and why it's wrong in as specific a manner as possible to give later programmers a chance.

>      private:
>          virtual ConstructType getConstructData(ConstructData&);
>          virtual CallType getCallData(CallData&);
>  
> +        virtual void markChildren(MarkStack&);
> +
>          virtual const ClassInfo* classInfo() const { return &info; }
>  
>          RefPtr<Structure> m_errorStructure;
> +
> +    protected:
> +        static const unsigned StructureFlags = InternalFunction::StructureFlags | OverridesMarkChildren;

Normally we would put a protected member like this above the private members, unless there's a good reason not to.

> +        JSValue const &storedPrototype() const { return m_prototype; }
> +        JSValue &storedPrototype() { return m_prototype; }

The "&" characters should be next to the type name and const, not the member name.

This is really close to a review+, but I'll say review- so you can fix one or more of the things I mentioned above.
Comment 18 Nathan Lawrence 2010-07-29 13:47:21 PDT
Created attachment 62985 [details]
Patch

Added in Darin's suggestions except changing reg to register.  At least within the jit code, reg seems to be the accepted variable name for registers.
Comment 19 Darin Adler 2010-07-29 14:01:01 PDT
Comment on attachment 62985 [details]
Patch

> -                markStack.append(reinterpret_cast<JSCell*>(xAsBits));
> +                // While markStack.append normally takes a reference to update,
> +                // we don't actually want the heap to be updated since we don't
> +                // know for sure that it's actually a pointer.  In the future
> +                // this will be replaced by some appendRoot function for this
> +                // specific case.
> +                JSCell* cell = reinterpret_cast<JSCell*>(xAsBits);
> +                markStack.append(cell);

Thanks. I find this comment really helpful. We normally use one space after periods, not two.

> +        // In this case, we need to be able to change the pointer, and although we know
> +        // it to be safe, C++ doesn't, requiring us to use templated functions that
> +        // pass a casted version to an internal function.

It's too bad you didn't give the reason we know it to be safe. Otherwise excellent!

> +    ALWAYS_INLINE void MarkStack::append(Register& reg)

Hi, Reg! I guess it's short for Reginald ;-)

>  #include "JSValue.h"
> +#include "Register.h"
>  #include <wtf/Noncopyable.h>

Why are you adding an include of Register.h to MarkStack.h? I don't see anything in this patch that requires that.

r=me

Please see if you can land this without adding that Register.h include.
Comment 20 Nathan Lawrence 2010-08-02 11:18:17 PDT
Created attachment 63240 [details]
Patch

Added Darin's suggestions + remembered to add the necessary change to JavaScriptGlue.
Comment 21 Darin Adler 2010-08-02 11:52:05 PDT
Comment on attachment 63240 [details]
Patch

> +            struct {
> +                int32_t tag;
> +                JSCell* m_ptr;
> +            } asPtr;

> +            struct {
> +                JSCell* m_ptr;
> +                int32_t tag;
> +            } asPtr;

Naming the pointer m_ptr instead of ptr or pointer seems to violate the prevailing style here.

> +        Removed unneeded marking.  We need to remove this marking in order to have
> +        MarkStack::append take references for updating movable objects.
> +        https://bugs.webkit.org/show_bug.cgi?id=41177

This answers why we need to remove the marking, but doesn't say why the marking is unneeded.

>  void JSValueWrapper::JSObjectMark(void *data)
>  {
> -    JSValueWrapper* ptr = (JSValueWrapper*)data;
> -    if (ptr)
> -    {
> -        // This results in recursive marking but will be otherwise safe and correct.
> -        // We claim the array vptr is 0 because we don't have access to it here, and
> -        // claiming 0 is functionally harmless -- it merely means that we can't
> -        // devirtualise marking of arrays when recursing from this point.
> -        MarkStack markStack(0);
> -        markStack.append(ptr->fValue.get());
> -        markStack.drain();
> -    }
>  }

It's a little strange to leave this function around with an argument, but not doing any work. I think the comment about why marking is unneeded could probably go here in the code rather than just in the change log.

review- because I think m_ptr is not the right way to name that
Comment 22 Nathan Lawrence 2010-08-02 12:14:16 PDT
Created attachment 63245 [details]
Patch
Comment 23 WebKit Commit Bot 2010-08-04 10:18:52 PDT
Comment on attachment 63245 [details]
Patch

Clearing flags on attachment: 63245

Committed r64655: <http://trac.webkit.org/changeset/64655>
Comment 24 WebKit Commit Bot 2010-08-04 10:18:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2010-08-04 10:45:30 PDT
http://trac.webkit.org/changeset/64655 might have broken Qt Linux Release
Comment 26 Csaba Osztrogonác 2010-08-05 00:56:35 PDT
Reopened, because it was rolled out by http://trac.webkit.org/changeset/64684
Comment 27 Csaba Osztrogonác 2010-08-05 02:22:03 PDT
On Qt I reproduced the crash:

$WebKitTools/Scripts/run-webkit-tests --debug fast/js/sputnik/Conformance --exit-after-n-failures 1

...
fast/js/sputnik/Conformance/09_Type_Conversion/9.2_ToBoolean/S9.2_A4_T1.html -> crashed

Exiting early after 1 failures. 593 tests run.

$ gdb WebKitBuild/Debug/bin/DumpRenderTree core

(gdb) bt
#0  0xf5afe6c2 in JSC::JSValue::tag (this=0x8) at ../../../JavaScriptCore/runtime/JSValue.h:584
#1  0xf5aff881 in JSC::JSValue::operator bool (this=0x8) at ../../../JavaScriptCore/runtime/JSValue.h:523
#2  0xf5eca527 in JSC::MarkStack::append (this=0x81940a8, value=@0x8) at ../../../JavaScriptCore/runtime/JSCell.h:361
#3  0xf6ad7387 in JSC::NativeErrorConstructor::markChildren (this=0xe92ff980, markStack=@0x81940a8) at ../../../JavaScriptCore/runtime/NativeErrorConstructor.cpp:80
#4  0xf6a879e4 in JSC::MarkStack::markChildren (this=0x81940a8, cell=0xe92ff980) at ../../../JavaScriptCore/runtime/JSArray.h:230
#5  0xf6a87d08 in JSC::MarkStack::drain (this=0x81940a8) at ../../../JavaScriptCore/runtime/JSArray.h:272
#6  0xf6a8022d in JSC::Heap::markConservatively (this=0x8193fb4, markStack=@0x81940a8, start=0xffffb76c, end=0xffffe000) at ../../../JavaScriptCore/runtime/Collector.cpp:701
#7  0xf6a804f4 in JSC::Heap::markCurrentThreadConservativelyInternal (this=0x8193fb4, markStack=@0x81940a8) at ../../../JavaScriptCore/runtime/Collector.cpp:712
#8  0xf6a8052b in JSC::Heap::markCurrentThreadConservatively (this=0x8193fb4, markStack=@0x81940a8) at ../../../JavaScriptCore/runtime/Collector.cpp:734
#9  0xf6a8054c in JSC::Heap::markStackObjectsConservatively (this=0x8193fb4, markStack=@0x81940a8) at ../../../JavaScriptCore/runtime/Collector.cpp:886
#10 0xf6a806a5 in JSC::Heap::markRoots (this=0x8193fb4) at ../../../JavaScriptCore/runtime/Collector.cpp:1026
#11 0xf6a8119f in JSC::Heap::reset (this=0x8193fb4) at ../../../JavaScriptCore/runtime/Collector.cpp:1163
#12 0xf6a814a6 in JSC::Heap::allocate (this=0x8193fb4, s=32) at ../../../JavaScriptCore/runtime/Collector.cpp:337
#13 0xf5b1b47f in JSC::JSCell::operator new (size=32, globalData=0x8193258) at ../../../JavaScriptCore/runtime/JSCell.h:171
#14 0xf5b1c8d7 in JSC::jsString (globalData=0x8193258, s=@0xffffbf00) at ../../../JavaScriptCore/runtime/JSString.h:513
#15 0xf5b1c939 in JSC::jsString (exec=0x82256b4, s=@0xffffbf00) at ../../../JavaScriptCore/runtime/JSString.h:552
#16 0xf6ad7de2 in NativeErrorPrototype (this=0xe92ffa00, exec=0x82256b4, globalObject=0xe92fc580, structure={m_ptr = 0xffffb9b8}, nameAndMessage=@0xffffbf00, constructor=0xe92ff980)
    at ../../../JavaScriptCore/runtime/NativeErrorPrototype.cpp:38
#17 0xf6ad7738 in NativeErrorConstructor (this=0xe92ff980, exec=0x82256b4, globalObject=0xe92fc580, structure={m_ptr = 0xffffbef8}, prototypeStructure={m_ptr = 0xffffbefc}, nameAndMessage=@0xffffbf00)
    at ../../../JavaScriptCore/runtime/NativeErrorConstructor.cpp:38
#18 0xf6ab9e52 in JSC::JSGlobalObject::reset (this=0xe92fc580, prototype={u = {asEncodedJSValue = -4677712576, asDouble = -nan(0xffffee92fc540), asBits = {payload = -382745280, tag = -2}, asPtr = {ptr = 0xe92fc540, tag = -2}}})
    at ../../../JavaScriptCore/runtime/JSGlobalObject.cpp:266
#19 0xf6abcd21 in JSC::JSGlobalObject::init (this=0xe92fc580, thisValue=0xe9280000) at ../../../JavaScriptCore/runtime/JSGlobalObject.cpp:149
#20 0xf5eca8fb in JSGlobalObject (this=0xe92fc580, structure={m_ptr = 0xffffc118}, data=0x8225660, thisValue=0xe9280000) at ../../../JavaScriptCore/runtime/JSGlobalObject.h:186
#21 0xf5eca9fe in JSDOMGlobalObject (this=0xe92fc580, structure={m_ptr = 0xffffc1b0}, data=0x8225660, thisValue=0xe9280000) at ../../../WebCore/bindings/js/JSDOMGlobalObject.cpp:46
#22 0xf5ed20cf in JSDOMWindowBase (this=0xe92fc580, structure={m_ptr = 0xffffc208}, window={m_ptr = 0xffffc204}, shell=0xe9280000) at ../../../WebCore/bindings/js/JSDOMWindowBase.cpp:56
#23 0xf5ca4a0a in JSDOMWindow (this=0xe92fc580, structure={m_ptr = 0xffffc284}, impl={m_ptr = 0xffffc28c}, shell=0xe9280000) at generated/JSDOMWindow.cpp:958
#24 0xf5edc9cd in WebCore::JSDOMWindowShell::setWindow (this=0xe9280000, domWindow={m_ptr = 0xffffc300}) at ../../../WebCore/bindings/js/JSDOMWindowShell.cpp:67
#25 0xf5f1bbbd in WebCore::ScriptController::clearWindowShell (this=0x8167970, goingIntoPageCache=false) at ../../../WebCore/bindings/js/ScriptController.cpp:204
#26 0xf63dd50a in WebCore::FrameLoader::clear (this=0x81676e0, clearWindowProperties=true, clearScriptObjects=true, clearFrameView=true) at ../../../WebCore/loader/FrameLoader.cpp:644
#27 0xf63cf9c6 in WebCore::DocumentWriter::begin (this=0x81677cc, url=@0x8167848, dispatch=false, origin=0x0) at ../../../WebCore/loader/DocumentWriter.cpp:125
#28 0xf63dcfa3 in WebCore::FrameLoader::receivedFirstData (this=0x81676e0) at ../../../WebCore/loader/FrameLoader.cpp:674
#29 0xf63dd396 in WebCore::FrameLoader::willSetEncoding (this=0x81676e0) at ../../../WebCore/loader/FrameLoader.cpp:1144
#30 0xf63ce995 in WebCore::DocumentWriter::setEncoding (this=0x81677cc, name=@0x816403c, userChosen=false) at ../../../WebCore/loader/DocumentWriter.cpp:236
#31 0xf675622f in WebCore::FrameLoaderClientQt::committedLoad (this=0x8163fe8, loader=0x81e56f0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147) at ../../../WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:797
#32 0xf63d88d4 in WebCore::FrameLoader::committedLoad (this=0x81676e0, loader=0x81e56f0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147) at ../../../WebCore/loader/FrameLoader.cpp:2761
#33 0xf63c70e5 in WebCore::DocumentLoader::commitLoad (this=0x81e56f0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147) at ../../../WebCore/loader/DocumentLoader.cpp:280
#34 0xf63c7174 in WebCore::DocumentLoader::receivedData (this=0x81e56f0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147) at ../../../WebCore/loader/DocumentLoader.cpp:292
#35 0xf63db49d in WebCore::FrameLoader::receivedData (this=0x81676e0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147) at ../../../WebCore/loader/FrameLoader.cpp:1563
#36 0xf6409a08 in WebCore::MainResourceLoader::addData (this=0x83cccf0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147, allAtOnce=false) at ../../../WebCore/loader/MainResourceLoader.cpp:149
#37 0xf64182f6 in WebCore::ResourceLoader::didReceiveData (this=0x83cccf0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147, lengthReceived=2147, allAtOnce=false) at ../../../WebCore/loader/ResourceLoader.cpp:260
#38 0xf6408bcf in WebCore::MainResourceLoader::didReceiveData (this=0x83cccf0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147, lengthReceived=2147, allAtOnce=false) at ../../../WebCore/loader/MainResourceLoader.cpp:420
#39 0xf64177fa in WebCore::ResourceLoader::didReceiveData (this=0x83cccf0,
    data=0x82d7fa0 "<html>\n<head>\n<meta charset='utf-8'>\n<style>\n.pass {\n    font-weight: bold;\n    color: green;\n}\n.fail {\n    font-weight: bold;\n    color: red;\n}\n</style>\n\n<script>\nif (window.layoutTestController)\n   "..., length=2147, lengthReceived=2147) at ../../../WebCore/loader/ResourceLoader.cpp:431
#40 0xf67183b9 in WebCore::QNetworkReplyHandler::forwardData (this=0x81c7b00) at ../../../WebCore/platform/network/qt/QNetworkReplyHandler.cpp:407
---Type <return> to continue, or q <return> to quit---
#41 0xf671a49d in WebCore::QNetworkReplyHandler::qt_metacall (this=0x81c7b00, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x833d310) at ./moc_QNetworkReplyHandler.cpp:86
#42 0xf3d27435 in QMetaObject::metacall (object=0xe9259000, cl=QMetaObject::InvokeMetaMethod, idx=7, argv=0x833d310) at kernel/qmetaobject.cpp:237
#43 0xf3d31a36 in QMetaCallEvent::placeMetaCall (this=0x82781c0, object=0x81c7b00) at kernel/qobject.cpp:561
#44 0xf3d32fc3 in QObject::event (this=0x81c7b00, e=0x82781c0) at kernel/qobject.cpp:1240
#45 0xf3f930dc in QApplicationPrivate::notify_helper (this=0x811e990, receiver=0x81c7b00, e=0x82781c0) at kernel/qapplication.cpp:4300
#46 0xf3f99b22 in QApplication::notify (this=0xffffcfa4, receiver=0x81c7b00, e=0x82781c0) at kernel/qapplication.cpp:3704
#47 0xf3d21feb in QCoreApplication::notifyInternal (this=0xffffcfa4, receiver=0x81c7b00, event=0x82781c0) at kernel/qcoreapplication.cpp:704
#48 0xf3d22f4f in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x813c2e0) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:215
#49 0xf3d230fd in QCoreApplication::sendPostedEvents (receiver=0x0, event_type=0) at kernel/qcoreapplication.cpp:1238
#50 0xf3d4ec5f in postEventSourceDispatch (s=0x8120e50) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:220
#51 0xf321f1d8 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#52 0xf3222873 in ?? () from /usr/lib/libglib-2.0.so.0
#53 0x08145d80 in ?? ()
#54 0x00000000 in ?? ()
Comment 28 Zoltan Herczeg 2010-08-05 05:32:15 PDT
We found two issues with this patch, but not sure how to fix them:

Issue 1) AlignedMemoryAllocator.h

WTF::Bitmap<bitmapSize> m_bitmap; is not cleared at the constructor, thus its initialization value is a memory garbage. Sometimes all bits are set to 1, and AlignedMemoryAllocator<blockSize>::allocate() fails, since m_bitmap.isFull() is true.

I can easly fix this with a m_bitmap.clearAll(), but I am not sure it is good to have a class without a default constructor. I feel the best would be the following constructor:

WTF::Bitmap::Bitmap(bool needsInitialization = true) {
    if (needsInitialization)
        clearAll();
}

We could still keep the performance advantage of uninitialized WTF::Bitmaps in some cases (not sure there are such cases), but we could not forget the initialization otherwise.

Issue 2) NativeErrorConstructor.cpp constructor

NativeErrorConstructor is a JSCell, which also allocates another JSCell in its constructor:

NativeErrorPrototype* prototype = new (exec) NativeErrorPrototype(...);

At this point, the virtual pointer (vptr) of NativeErrorConstructor is copied to the cell buffer, but the object itself is NOT initialized at this point. If the "new NativeErrorPrototype" calls the garbage collector (it is possible, since it allocates a new cell), the NativeErrorConstructor::markChildren called for an uninitialized NativeErrorConstructor, which leads to a segmentation fault.

There are several possible solutions for this bug, like checking m_errorStructure in markChildren (setting it to NULL before "new NativeErrorPrototype") or passing the "new NativeErrorPrototype" to the constructor as an argument.

I want to fix these bugs myself, but I would like to hear your opinion before I submit patches.
Comment 29 Zoltan Herczeg 2010-08-05 05:41:07 PDT
(In reply to comment #28)
> We found two issues with this patch, but not sure how to fix them:

Ok, not the patch itself. This patch can trigger other, hidden bugs.
Comment 30 Eric Seidel (no email) 2010-08-05 08:57:04 PDT
Comment on attachment 62985 [details]
Patch

Cleared Darin Adler's review+ from obsolete attachment 62985 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 31 Nathan Lawrence 2010-08-05 09:00:28 PDT
(In reply to comment #28)
> We found two issues with this patch, but not sure how to fix them:
> 
> Issue 1) AlignedMemoryAllocator.h

Great catch here.  I definitely agree that it should be in the constructor.

> Issue 2) NativeErrorConstructor.cpp constructor

Both options are good.  We could also put m_errorStructure with the construction of the NativeErrorPrototype object in the initializer list since there doesn't seem to be a reason why the puts need to happen before m_errorStructure is initialized.
Comment 32 Zoltan Herczeg 2010-08-09 05:41:57 PDT
I suggest that m_errorStructure should be set to 0 before "new (exec) NativeErrorPrototype" and this could be tested in markChildren. This could be part of the patch above, since markChildren has not yet implemented.

A constructor -> create change would require far more changes, but I could do that if you think that is better.