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

[Sam Magnuson]

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

[Sam Magnuson]

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

[Sam Magnuson]

[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]

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

[Sam Magnuson]

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

[Jędrzej Nowacki]

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]

(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]

Created attachment 59429 [details]
Rediff against trunk

[Simon Hausmann]

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]

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

[Sam Magnuson]

Created attachment 59738 [details]
Patch

[Sam Magnuson]

(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 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]

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

[Andreas Kling]

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

[Andreas Kling]

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