[webkit-reviews] review denied: [Bug 65288] Add checks to ensure allocation does not take place during initialization of GC-managed objects : [Attachment 102790] Patch with allocation refactorings

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 65288: Add checks to ensure allocation does not take place during
initialization of GC-managed objects
https://bugs.webkit.org/show_bug.cgi?id=65288

Attachment 102790: Patch with allocation refactorings
https://bugs.webkit.org/attachment.cgi?id=102790&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.