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

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.