Bug 64999 - Remove operator new from JSCell
Summary: Remove operator new from JSCell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 64732
Blocks: 65288
  Show dependency treegraph
 
Reported: 2011-07-21 17:10 PDT by Mark Hahnenberg
Modified: 2011-07-27 21:59 PDT (History)
3 users (show)

See Also:


Attachments
Patch (40.72 KB, patch)
2011-07-27 10:33 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (51.74 KB, patch)
2011-07-27 12:46 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (52.03 KB, patch)
2011-07-27 13:10 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (52.48 KB, patch)
2011-07-27 15:19 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (52.50 KB, patch)
2011-07-27 15:27 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-07-21 17:10:02 PDT
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.
Comment 1 Mark Hahnenberg 2011-07-27 10:33:12 PDT
Created attachment 102156 [details]
Patch
Comment 2 Darin Adler 2011-07-27 10:36:31 PDT
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?
Comment 3 Mark Hahnenberg 2011-07-27 10:42:03 PDT
> > 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.
Comment 4 Early Warning System Bot 2011-07-27 10:46:45 PDT
Comment on attachment 102156 [details]
Patch

Attachment 102156 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9260004
Comment 5 Mark Hahnenberg 2011-07-27 10:54:01 PDT
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.
Comment 6 Oliver Hunt 2011-07-27 10:58:10 PDT
(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_)
Comment 7 Mark Hahnenberg 2011-07-27 10:59:28 PDT
> 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.
Comment 8 Mark Hahnenberg 2011-07-27 12:46:47 PDT
Created attachment 102172 [details]
Patch
Comment 9 Early Warning System Bot 2011-07-27 13:03:52 PDT
Comment on attachment 102172 [details]
Patch

Attachment 102172 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9247893
Comment 10 Darin Adler 2011-07-27 13:07:37 PDT
EWS says Qt fails to build.
Comment 11 Mark Hahnenberg 2011-07-27 13:10:41 PDT
Created attachment 102175 [details]
Patch
Comment 12 Darin Adler 2011-07-27 14:35:49 PDT
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?
Comment 13 Mark Hahnenberg 2011-07-27 15:19:22 PDT
Created attachment 102193 [details]
Patch
Comment 14 Oliver Hunt 2011-07-27 15:24:00 PDT
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
Comment 15 Mark Hahnenberg 2011-07-27 15:27:45 PDT
Created attachment 102196 [details]
Patch
Comment 16 WebKit Review Bot 2011-07-27 21:59:25 PDT
Comment on attachment 102196 [details]
Patch

Clearing flags on attachment: 102196

Committed r91903: <http://trac.webkit.org/changeset/91903>
Comment 17 WebKit Review Bot 2011-07-27 21:59:31 PDT
All reviewed patches have been landed.  Closing bug.