Bug 64999 – Remove operator new from JSCell

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

[Mark Hahnenberg]

This is the third installment of the ongoing effort to phase out the use of constructors for GC-managed objects in favor of static create methods.  We want to avoid calling operator new any more, so the best way to do that is to remove it.  Actually, the best way is to assert that we never use it.  If we fully remove the calls and somebody uses them or if a call was missed during the refactoring process we can get strange behavior in the browser that is difficult to track down to the source.  By leaving an assert in, it is easy to determine exactly where in the code the offending call to new took place.

[Mark Hahnenberg]

Created attachment 102156 [details]
Patch

[Darin Adler]

Comment on attachment 102156 [details]
Patch

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

> Source/JavaScriptCore/debugger/DebuggerActivation.h:37
> +        static DebuggerActivation* create(JSGlobalData& globalData, JSObject* obj)

We use words like “object”, not abbreviations like “obj”.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:130
> +        static JS_EXPORTDATA const ClassInfo s_info;

Why are we making this public?

[Mark Hahnenberg]

> > Source/JavaScriptCore/runtime/JSGlobalObject.h:130
> > +        static JS_EXPORTDATA const ClassInfo s_info;
> 
> Why are we making this public?

It was public before.  The way the diff worked out made it look like I changed it.

[Early Warning System Bot]

Comment on attachment 102156 [details]
Patch

Attachment 102156 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9260004

[Mark Hahnenberg]

Should I put a comment on the operator new declarations in JSCell saying that there are no concrete implementations to prevent successful linking? This would prevent somebody going and looking for them in the future and wasting a lot of time.

[Oliver Hunt]

(In reply to comment #5)
> Should I put a comment on the operator new declarations in JSCell saying that there are no concrete implementations to prevent successful linking? This would prevent somebody going and looking for them in the future and wasting a lot of time.

Yeah, that probably makes sense.

Also look into why the Qt build is failing (_sigh_)

[Mark Hahnenberg]

> Yeah, that probably makes sense.

Will do.

> Also look into why the Qt build is failing (_sigh_)

I didn't actually test any of the builds on the other platforms yet.  I didn't expect the patch to clear the build bots and the commit queue in its current state.  Removing operator new from JSCell affects pretty much every platform, so I'm using the bots to catch the build errors more or less simultaneously.

[Mark Hahnenberg]

Created attachment 102172 [details]
Patch

[Early Warning System Bot]

Comment on attachment 102172 [details]
Patch

Attachment 102172 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9247893

[Darin Adler]

EWS says Qt fails to build.

[Mark Hahnenberg]

Created attachment 102175 [details]
Patch

[Darin Adler]

Comment on attachment 102175 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCell.h:127
> +        // Note that the first two declarations of operator new have no corresponding implementation and 
> +        // will cause link errors if you use them.
>          void* operator new(size_t, ExecState*);
>          void* operator new(size_t, JSGlobalData*);

Can these be made private so we are more likely to get compilation errors rather than linking errors?

[Mark Hahnenberg]

Created attachment 102193 [details]
Patch

[Oliver Hunt]

Comment on attachment 102193 [details]
Patch

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

noticed a couple of additional abbreviations, correct those and this will be finished :D

> Source/WebCore/bridge/c/CRuntimeObject.h:40
> +    static CRuntimeObject* create(ExecState* exec, JSGlobalObject* globalObject, PassRefPtr<CInstance> cinst)

normal name for 'cinst' would be instance

> Source/WebCore/bridge/objc/objc_runtime.h:95
> +    static ObjcFallbackObjectImp* create(ExecState* exec, JSGlobalObject* globalObject, ObjcInstance* inst, const Identifier& propertyName)

s/inst/instance

> Source/WebKit/mac/Plugins/Hosted/ProxyRuntimeObject.h:39
> +    static ProxyRuntimeObject* create(JSC::ExecState* exec, JSC::JSGlobalObject* globalObject, PassRefPtr<ProxyInstance> inst)

s/inst/instance

[Mark Hahnenberg]

Created attachment 102196 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102196 [details]
Patch

Clearing flags on attachment: 102196

Committed r91903: <http://trac.webkit.org/changeset/91903>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.