Bug 65288 – Add checks to ensure allocation does not take place during initialization of GC-managed objects

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

This is the fourth bug in a series of refactorings to improve the safety of the creation of GC-managed objects in JSC.  Since we do all object creation inside static create methods, we can now add a flag to JSGlobalData that can be checked to ensure that we're not performing allocation during initialization.

[Mark Hahnenberg]

Created attachment 102278 [details]
Patch

[Mark Hahnenberg]

Created attachment 102309 [details]
Patch

[Oliver Hunt]

Comment on attachment 102309 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102309&action=review

Use an RAII object for the setInitializingObject(false/true) calls

> Source/JavaScriptCore/runtime/JSGlobalData.h:232
> +#if ENABLE(GC_VALIDATION)
> +            m_initializingObject = value; 
> +#else
> +#endif

add UNUSED_PARAM(value) to the else block

[Mark Hahnenberg]

(In reply to comment #3)
> (From update of attachment 102309 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102309&action=review
> 
> Use an RAII object for the setInitializingObject(false/true) calls
I'm not sure what you mean by this.  Eventually we'd like the calls to be contained only within allocateCell and validateAlloc.  RAII objects created within the former would then need to be destroyed in the latter, making it rather awkward to persist them across the two calls without adding them to every single static create method.  Perhaps I'm just not understanding?

[Mark Hahnenberg]

Created attachment 102790 [details]
Patch with allocation refactorings

This patch adds the validateAlloc call as well as the initializingObject flag to JSGlobalData, and it refactors most of the allocation done within constructors do being done within the static create methods.

[Darin Adler]

Comment on attachment 102790 [details]
Patch with allocation refactorings

View in context: https://bugs.webkit.org/attachment.cgi?id=102790&action=review

Too much in one patch. Land the fixes first, simple ones all at once and then any complex ones separately on their own, then finally land the new checking machinery alone with no behavior changes.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1396
> +        StructureChain* prototypeChain = structure->prototypeChain(callFrame);
>          vPC[0] = getOpcode(op_put_by_id_transition);
>          vPC[4].u.structure.set(globalData, owner, structure->previousID());
>          vPC[5].u.structure.set(globalData, owner, structure);
> -        vPC[6].u.structureChain.set(callFrame->globalData(), codeBlock->ownerExecutable(), structure->prototypeChain(callFrame));
> +        vPC[6].u.structureChain.set(callFrame->globalData(), codeBlock->ownerExecutable(), prototypeChain);

How does this change relate to the rest of the patch? Can it be landed separately first?

> Source/JavaScriptCore/runtime/ArrayConstructor.cpp:55
>  ArrayConstructor::ArrayConstructor(ExecState* exec, JSGlobalObject* globalObject, Structure* structure, ArrayPrototype* arrayPrototype)
> -    : InternalFunction(&exec->globalData(), globalObject, structure, Identifier(exec, arrayPrototype->classInfo()->className))
> +    : InternalFunction(globalObject, structure)

I think we should land fixes for bugs found by the new mechanism first, separately. We can then land the new mechanism all by itself with no bug fixes or changes in behavior bundled with it.

[Darin Adler]

(In reply to comment #6)
> Too much in one patch. Land the fixes first, simple ones all at once and then any complex ones separately on their own, then finally land the new checking machinery alone with no behavior changes.

Or if you want to land the mechanism first then:

- First set of patches adds the mechanism, turned off.
- Next set of patches fixes the problems uncovered by the mechanism, in small pieces that are easy to review.
- Last patch turns on the checking.

[Mark Hahnenberg]

> > Source/JavaScriptCore/interpreter/Interpreter.cpp:1396
> > +        StructureChain* prototypeChain = structure->prototypeChain(callFrame);
> >          vPC[0] = getOpcode(op_put_by_id_transition);
> >          vPC[4].u.structure.set(globalData, owner, structure->previousID());
> >          vPC[5].u.structure.set(globalData, owner, structure);
> > -        vPC[6].u.structureChain.set(callFrame->globalData(), codeBlock->ownerExecutable(), structure->prototypeChain(callFrame));
> > +        vPC[6].u.structureChain.set(callFrame->globalData(), codeBlock->ownerExecutable(), prototypeChain);
> 
> How does this change relate to the rest of the patch? Can it be landed separately first?

Yep, just something I ran into while I was testing.  Created a bug for it: https://bugs.webkit.org/show_bug.cgi?id=65638

[Mark Hahnenberg]

(In reply to comment #7)
> (In reply to comment #6)
> > Too much in one patch. Land the fixes first, simple ones all at once and then any complex ones separately on their own, then finally land the new checking machinery alone with no behavior changes.
> 
> Or if you want to land the mechanism first then:
> 
> - First set of patches adds the mechanism, turned off.
> - Next set of patches fixes the problems uncovered by the mechanism, in small pieces that are easy to review.
> - Last patch turns on the checking.

I think it would be better to land the refactoring of the allocations first, since landing the mechanism first and then turning it on would break everything unless every single place that allocation occurred inside of a constructor were immediately pulled out of the constructors or guarded by first clearing the flag and then resetting it.

[Mark Hahnenberg]

> I think it would be better to land the refactoring of the allocations first, since landing the mechanism first and then turning it on would break everything unless every single place that allocation occurred inside of a constructor were immediately pulled out of the constructors or guarded by first clearing the flag and then resetting it.

Nevermind, I'm an idiot.  That's exactly what you said.

[Mark Hahnenberg]

Created attachment 102861 [details]
Refactoring of JavaScriptCore

[Mark Hahnenberg]

Created attachment 102949 [details]
JSC patch with Windows build fixes

[Mark Hahnenberg]

Created attachment 102951 [details]
Fixing windows again

[Mark Hahnenberg]

Created attachment 102964 [details]
Fixing windows again

[Mark Hahnenberg]

Created attachment 102977 [details]
Fixing windows again

[Oliver Hunt]

Comment on attachment 102977 [details]
Fixing windows again

One thing i have a problem with is the inconsistency of initProperties() vs. inlining initialisation into the create() method.  The inlining of anything into a create method potentially breaks subclassing.

I think a much better approach would be to
* Give JSCell an initProperties() method
* require every initProperties method to do BaseObject::initProperties(...) first
* Then every create() method can follow the pattern
    JSFoo* foo = new (allocateCell<JSFoo>(*exec->heap())) JSFoo(...);
    foo->initProperties(...);
    return foo;
  Then we can also add an assertion to the base JSCell::initProperties to ensure we have actually completed initialisation if we wanted to.  But even if we don't do that, we can look at create methods, and easily see obvious errors.

[Mark Hahnenberg]

Created attachment 104562 [details]
New, much smaller patch.

[Darin Adler]

Comment on attachment 104562 [details]
New, much smaller patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=104562&action=review

> Source/JavaScriptCore/runtime/JSCell.h:174
> +        void constructorBody(JSGlobalData& globalData)
> +        {
> +#if ENABLE(GC_VALIDATION)
> +            ASSERT(globalData.isInitializingObject());
> +            globalData.setInitializingObject(false);
> +#else
> +            UNUSED_PARAM(globalData);
> +#endif
> +            ASSERT(m_structure);
> +        }
> +
> +        void constructorBody(JSGlobalData& globalData, Structure* structure, CreatingEarlyCellTag)
> +        {
> +#if ENABLE(GC_VALIDATION)
> +            globalData.setInitializingObject(false);
> +            if (structure)
> +#endif
> +                m_structure.setEarlyValue(globalData, this, structure);
> +            // Very first set of allocations won't have a real structure.
> +            ASSERT(m_structure || !globalData.structureStructure);
> +        }

I don’t see any benefit in putting these inside a separate “constructorBody” function. These should just go inside the constructor’s body! That would eliminate the UNUSED_PARAM, for one thing.

> Source/JavaScriptCore/runtime/JSGlobalData.h:298
> +        bool isInitializingObject() const 
> +        {
> +#if ENABLE(GC_VALIDATION)
> +            return m_initializingObject;
> +#else
> +            return false;
> +#endif
> +        }
> +
> +        void setInitializingObject(bool initializingObject)
> +        {
> +#if ENABLE(GC_VALIDATION)
> +            m_initializingObject = initializingObject;
> +#else
> +            UNUSED_PARAM(initializingObject);
> +#endif
> +        }

It would be cleaner to put just the declaration here in the class and have two completely separate definitions below with the ifdefs.

I also think it would be cleaner to not even have the isInitializingObject() function exist if GC_VALIDATION is off, because false is not the correct answer, and it’s unlikely we can write code that uses that.

> Source/JavaScriptCore/runtime/JSGlobalData.h:308
> +        bool m_initializingObject;

Shouldn’t this be inside an #if ENABLE(GC_VALIDATION)?

> Source/WebCore/bindings/js/JSDOMWindowShell.h:41
> -        typedef JSC::JSNonFinalObject Base;
>      public:
> +        typedef JSC::JSNonFinalObject Base;

It’s dangerous for someone to accidentally use this in a derived class, so it’s not good to have it public. Maybe we can friend operator new instead?

> Source/WebKit/mac/Plugins/Hosted/ProxyRuntimeObject.h:45
> +        // FIXME: deprecatedGetDOMStructure uses the prototype off of the wrong global object
> +        // exec-globalData() is also likely wrong.

This is two sentences so probably needs two periods.

[Oliver Hunt]

> I don’t see any benefit in putting these inside a separate “constructorBody” function. These should just go inside the constructor’s body! That would eliminate the UNUSED_PARAM, for one thing.

The whole point of this series of patches is to kill off any real work (longer term: any work at all) in constructors, as it's not necessarily GC-safe to have side-effecting operations in your constructor.

[Darin Adler]

(In reply to comment #19)
> > I don’t see any benefit in putting these inside a separate “constructorBody” function. These should just go inside the constructor’s body! That would eliminate the UNUSED_PARAM, for one thing.
> 
> The whole point of this series of patches is to kill off any real work (longer term: any work at all) in constructors, as it's not necessarily GC-safe to have side-effecting operations in your constructor.

Yes. I understand that.

That goal has nothing to do with putting the bodies of two specific JSCell constructors into separate private JSCell functions. There is no reason to do that. All it does is make the code confusing. I think it’s a leftover from an earlier version of the patch where it had some purpose.

[Mark Hahnenberg]

> That goal has nothing to do with putting the bodies of two specific JSCell constructors into separate private JSCell functions. There is no reason to do that. All it does is make the code confusing. I think it’s a leftover from an earlier version of the patch where it had some purpose.

The goal is to have all objects who inherit from JSCell and who have non-trivial (i.e. non-empty) constructor bodies to have a corresponding constructorBody() method.  This will make it easier for people adding functionality that historically would have gone in the constructor body.  Instead of having a rule like "if you call a function that could potentially do GC allocation, it must go inside constructorBody()" we can just say "all code must go inside constructorBody()".

In this patch, the constructorBody() call seems like a one-off thing just for JSCell, but there was a recent patch (see: https://bugs.webkit.org/show_bug.cgi?id=66265) that added similar constructorBody() calls to a variety of classes in the JSCell hierarchy.  Eventually the calls will be moved out of their corresponding constructor bodies and will instead by called by the derived classes' implementations of constructorBody(), which will in turn be called by their static create() methods.  The constructorBody() in JSCell serves as the "termination point" for these future constructorBody() chains, and it also provides a convenient place to clear the initialization flag.  I agree it's a bit confusing now, but it will be much less confusing when all of this comes together :-)

[Mark Hahnenberg]

Created attachment 104870 [details]
Fixing review issues

[Darin Adler]

Comment on attachment 104870 [details]
Fixing review issues

View in context: https://bugs.webkit.org/attachment.cgi?id=104870&action=review

> Source/JavaScriptCore/runtime/JSGlobalData.h:296
> +        bool m_initializingObject;

Should be named m_isInitializingObject.

[Mark Hahnenberg]

Created attachment 104935 [details]
Fixing review issues

[WebKit Review Bot]

Comment on attachment 104935 [details]
Fixing review issues

Clearing flags on attachment: 104935

Committed r93688: <http://trac.webkit.org/changeset/93688>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.