40352 – [Qt] instance objects created for QObjects are somtimes GC'd

RESOLVED FIXED 40352

[Qt] instance objects created for QObjects are somtimes GC'd

https://bugs.webkit.org/show_bug.cgi?id=40352

Summary [Qt] instance objects created for QObjects are somtimes GC'd

Sam Magnuson

Reported 2010-06-08 23:23:47 PDT

I have an QObject (A) with a child object (B). B's only reference in the JS engine is through A (ie A.B). I've attached some JS properties (in fact function handlers) to B, and I'm finding that at some point B was getting GC'd. Upon investigation I found that I could create a global reference to B (ie var keep_b = A.B) and the problem would go away - however this is clearly error prone and probably not as intended. I've fixed qt_instance so that it will go about marking

Attachments
proposed patch to mark children/dynamic objects created through a qobject (4.65 KB, patch) 2010-06-08 23:32 PDT, Sam Magnuson	no flags	Details Formatted Diff Diff
proposed patch to mark children/dynamic objects created through a qobject (5.71 KB, patch) 2010-06-09 12:33 PDT, Sam Magnuson	no flags	Details Formatted Diff Diff
Proposed patch with proper coding style (5.67 KB, patch) 2010-06-12 10:19 PDT, Sam Magnuson	jedrzej.nowacki: review- jedrzej.nowacki: commit-queue-	Details Formatted Diff Diff
Rediff against trunk (2.91 KB, patch) 2010-06-22 16:08 PDT, Sam Magnuson	no flags	Details Formatted Diff Diff
Patch (8.84 KB, patch) 2010-06-24 23:25 PDT, Sam Magnuson	hausmann: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Sam Magnuson

Comment 1 2010-06-08 23:32:52 PDT

Created attachment 58215 [details] proposed patch to mark children/dynamic objects created through a qobject

Sam Magnuson

Comment 2 2010-06-08 23:34:26 PDT

[accidentally hit commit, cont'd] the attached patch will mark children/dynamic objects after they've been queried through a QtInstance so long as the child/dynamic property is available to QObject.

Sam Magnuson

Comment 3 2010-06-09 12:33:02 PDT

Created attachment 58272 [details] proposed patch to mark children/dynamic objects created through a qobject

Sam Magnuson

Comment 4 2010-06-12 10:19:38 PDT

Created attachment 58554 [details] Proposed patch with proper coding style

Jędrzej Nowacki

Comment 5 2010-06-15 05:33:57 PDT

Comment on attachment 58554 [details] Proposed patch with proper coding style The patch doesn't apply cq- There are a few unrelated changes and coding style issues (r-) there are no autotest. You can use check-webkit-style script to check some coding style issues. > + No new tests. (OOPS!) Please provide autotest. > #include "ArgList.h" > +#include "Error.h" > #include "JSDOMBinding.h" > #include "JSGlobalObject.h" > #include "JSLock.h" > +#include "ObjectPrototype.h" > +#include "PropertyNameArray.h" > #include "qt_class.h" > #include "qt_runtime.h" > -#include "PropertyNameArray.h" > #include "runtime_object.h" > -#include "ObjectPrototype.h" > -#include "Error.h" > - foreach(QtInstance* instance, cachedInstances.values(o)) > + foreach (QtInstance* instance, cachedInstances.values(o)) These changes are unrelated (there are more). Please cleanup the patch. > +#if 1 > +#endif This should be avoided. > + } else if ( m_object ) { A coding style issue.

Sam Magnuson

Comment 6 2010-06-15 09:15:28 PDT

(In reply to comment #5) > (From update of attachment 58554 [details]) > The patch doesn't apply cq- > There are a few unrelated changes and coding style issues (r-) > there are no autotest. > > You can use check-webkit-style script to check some coding style issues. > I did run check-webkit-style and when I fixed the issues flagged there, you r- it with unrelated changes. Is it the group preference that the old things that didn't pass check-webkit-style get left alone, and I have to ferret out the lines that I change from the output of check-webkit-style? That seems like more work than just fixing the coding style issues. > > + No new tests. (OOPS!) > Please provide autotest. > I'm not sure how to autotest this, it is related to a gc issue and not marking children. Can you point me to a test that is related that I can review? > > #include "ArgList.h" > > +#include "Error.h" > > #include "JSDOMBinding.h" > > #include "JSGlobalObject.h" > > #include "JSLock.h" > > +#include "ObjectPrototype.h" > > +#include "PropertyNameArray.h" > > #include "qt_class.h" > > #include "qt_runtime.h" > > -#include "PropertyNameArray.h" > > #include "runtime_object.h" > > -#include "ObjectPrototype.h" > > -#include "Error.h" > > > - foreach(QtInstance* instance, cachedInstances.values(o)) > > + foreach (QtInstance* instance, cachedInstances.values(o)) > These changes are unrelated (there are more). Please cleanup the patch. > I was fixing what check-webkit-style had flagged - I got the impression it was to pass that script with no errors. > > +#if 1 > > +#endif > This should be avoided. > Agree, will remove. > > + } else if ( m_object ) { > A coding style issue. Can you be more specific? check-webkit-style doesn't flag any issues on the file as it is.

Sam Magnuson

Comment 7 2010-06-22 16:08:19 PDT

Created attachment 59429 [details] Rediff against trunk

Simon Hausmann

Comment 8 2010-06-23 07:56:54 PDT

Comment on attachment 59429 [details] Rediff against trunk WebCore/ChangeLog:13 + No new tests. (OOPS!) If there are no new tests, then please remove this line. However there are some tests for this code in WebKit/qt/tests/qwebframe (don't ask :). Could you add an auto-tests for this case there? WebCore/bridge/qt/qt_instance.cpp:202 + } else if ( m_object ) { Please remove the spaces around m_object. WebCore/bridge/qt/qt_instance.cpp:212 + if ( mark ) { Ditto.

Simon Hausmann

Comment 9 2010-06-23 07:57:54 PDT

In general however I think this patch looks good (I forgot to mention that earlier)

Sam Magnuson

Comment 10 2010-06-24 23:25:16 PDT

Created attachment 59738 [details] Patch

Sam Magnuson

Comment 11 2010-06-24 23:26:33 PDT

(In reply to comment #9) > In general however I think this patch looks good (I forgot to mention that earlier) (In reply to comment #8) > (From update of attachment 59429 [details]) > WebCore/ChangeLog:13 > + No new tests. (OOPS!) > If there are no new tests, then please remove this line. > > However there are some tests for this code in WebKit/qt/tests/qwebframe (don't ask :). Could you add an auto-tests for this case there? > > WebCore/bridge/qt/qt_instance.cpp:202 > + } else if ( m_object ) { > Please remove the spaces around m_object. > > WebCore/bridge/qt/qt_instance.cpp:212 > + if ( mark ) { > Ditto. Thanks for the feedback, I've added a test - and fixed the coding style issues.

Simon Hausmann

Comment 12 2010-07-08 23:16:55 PDT

Comment on attachment 59738 [details] Patch r=me. Thanks for the extensive auto-test! The ChangeLog for WebKit/qt is missing, and there's a stray qDebug. I'll fix it up before landing.

Simon Hausmann

Comment 13 2010-07-08 23:27:21 PDT

Committed r62898: <http://trac.webkit.org/changeset/62898>

Andreas Kling

Comment 14 2010-07-18 19:24:48 PDT

This change <http://trac.webkit.org/changeset/62898> causes tst_QWebFrame to crash. Reopening.

Andreas Kling

Comment 15 2010-07-27 04:08:37 PDT

I opened bug 43039 for the regression introduced by this change.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component WebKit Qt

Assignee

QtWebKit Unassigned

Reported

2010-06-08 23:23 PDT

Modified

2010-07-27 04:08 PDT History

CC List

2 users Show

URL

Keywords Qt, QtTriaged

Depends on

Blocks