Bug 29351

Summary: Each wrapped Objective-C object should use a single RuntimeObjectImp
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit APIAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, ddkilzer, eric, ossy, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
patch
none
Patch v1
none
proposed patch none

Description Darin Adler 2009-09-17 16:34:22 PDT
When exposing Objective-C objects to JavaScript, we end up making multiple ObjcInstance objects and multiple RuntimeObjectImp objects for a single Objective-C object. This isn't good because it makes them receive multiple finalizeForWebScript calls.
Comment 1 Darin Adler 2009-09-17 16:34:38 PDT
rdar://problem/7142294
Comment 2 Darin Adler 2009-09-17 16:35:43 PDT
Created attachment 39735 [details]
patch
Comment 3 Sam Weinig 2009-09-17 19:23:40 PDT
Comment on attachment 39735 [details]
patch

r=me
Comment 4 Darin Adler 2009-09-18 10:49:23 PDT
http://trac.webkit.org/changeset/48513
Comment 5 Anders Carlsson 2009-09-18 12:29:31 PDT
Created attachment 39777 [details]
Patch v1
Comment 6 Anders Carlsson 2009-09-18 12:30:17 PDT
Comment on attachment 39777 [details]
Patch v1

Wrong bug, sorry!
Comment 7 David Kilzer (:ddkilzer) 2009-09-18 12:43:25 PDT
Comment on attachment 39735 [details]
patch

This one was landed.
Comment 8 Csaba Osztrogon√°c 2009-09-18 13:36:52 PDT
(In reply to comment #4)
> http://trac.webkit.org/changeset/48513

Unfortunately this patch brake Qt build: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/1693/steps/compile-webkit/logs/stdio

The error caused the new virtual function added to runtime.h,
but not defined in inherited class QtInstance : public Instance.
 	125 private:
 	126	    virtual RuntimeObjectImp* newRuntimeObject(ExecState*);

newRuntimeObject should have protected visibility, and createRuntimeObject should be renamed to newRuntimeObject in inherited class QtInstance.
Comment 9 Csaba Osztrogon√°c 2009-09-18 13:37:48 PDT
Created attachment 39785 [details]
proposed patch
Comment 10 Eric Seidel (no email) 2009-09-18 13:55:20 PDT
Comment on attachment 39785 [details]
proposed patch

LGTM.
Comment 11 Eric Seidel (no email) 2009-09-18 13:55:38 PDT
I have to re-open the bug for the commit-queue to see it.
Comment 12 Darin Adler 2009-09-18 14:44:54 PDT
(In reply to comment #8)
>      125 private:
>      126        virtual RuntimeObjectImp* newRuntimeObject(ExecState*);
> 
> newRuntimeObject should have protected visibility

Why? I don't agree. But it's not important.

> createRuntimeObject
> should be renamed to newRuntimeObject in inherited class QtInstance.

Yes, sorry! Thanks for fixing it.
Comment 13 WebKit Commit Bot 2009-09-18 15:12:25 PDT
Comment on attachment 39785 [details]
proposed patch

Clearing flags on attachment: 39785

Committed r48538: <http://trac.webkit.org/changeset/48538>
Comment 14 WebKit Commit Bot 2009-09-18 15:12:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Alexey Proskuryakov 2013-12-19 10:05:13 PST
This change introduced incorrect lifetime management and cross-origin data leaks in WebKit1, see bug 123029 comment 8.