Bug 29351 - Each wrapped Objective-C object should use a single RuntimeObjectImp
Summary: Each wrapped Objective-C object should use a single RuntimeObjectImp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-09-17 16:34 PDT by Darin Adler
Modified: 2013-12-19 10:05 PST (History)
6 users (show)

See Also:

Attachments
patch (17.91 KB, patch)
2009-09-17 16:35 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch v1 (6.60 KB, patch)
2009-09-18 12:29 PDT, Anders Carlsson 		no flags Details | Formatted Diff | Diff
proposed patch (1.54 KB, patch)
2009-09-18 13:37 PDT, Csaba Osztrogonác 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.