Bug 40352

Summary:

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

Product:

WebKit

Reporter:

Sam Magnuson <smagnuso>

Component:

WebKit Qt

Assignee:

QtWebKit Unassigned <webkit-qt-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hausmann, kling

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
proposed patch to mark children/dynamic objects created through a qobject	none
proposed patch to mark children/dynamic objects created through a qobject	none
Proposed patch with proper coding style	jedrzej.nowacki: review-, jedrzej.nowacki: commit-queue-
Rediff against trunk	none
Patch	hausmann: review+

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.