Bug 41177 – References for movable objects

• Status: REOPENED
• Component: JavaScriptCore

[Nathan Lawrence]

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.

[Mark Rowe (bdash)]

Is there more to this change than the ChangeLog entry?

[Geoffrey Garen]

Comment on attachment 59702 [details]
Patch

More patch please :).

[Nathan Lawrence]

Created attachment 59914 [details]
Patch (now with code!)

[Nathan Lawrence]

Created attachment 59916 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 59914 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3304932

[Early Warning System Bot]

Attachment 59916 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3324983

[WebKit Review Bot]

Attachment 59914 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3292967

[Geoffrey Garen]

Comment on attachment 59916 [details]
Patch

Angry builders :(.

[Nathan Lawrence]

Created attachment 59961 [details]
Patch

Should fix problems with 32 bit version.

[Early Warning System Bot]

Attachment 59961 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3283905

[WebKit Review Bot]

Attachment 59961 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3361009

[Nathan Lawrence]

Created attachment 59979 [details]
Patch (no, really it should compile this time...)

[Nathan Lawrence]

Created attachment 62960 [details]
Patch

Fixes how we mark NativeErrorConstructor

[Kent Hansen]

What's the further plan with this? Compacting GC?

[Darin Adler]

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.

[Nathan Lawrence]

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.

[Darin Adler]

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.

[Nathan Lawrence]

Created attachment 63240 [details]
Patch

Added Darin's suggestions + remembered to add the necessary change to JavaScriptGlue.

[Darin Adler]

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

[Nathan Lawrence]

Created attachment 63245 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 63245 [details]
Patch

Clearing flags on attachment: 63245

Committed r64655: <http://trac.webkit.org/changeset/64655>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64655 might have broken Qt Linux Release

[Csaba Osztrogonác]

Reopened, because it was rolled out by http://trac.webkit.org/changeset/64684

[Csaba Osztrogonác]

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 ?? ()

[Zoltan Herczeg]

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.

[Zoltan Herczeg]

(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.

[Eric Seidel (no email)]

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.

[Nathan Lawrence]

(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.

[Zoltan Herczeg]

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.