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: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Sam Magnuson
2010-06-08 23:23:47 PDT
Created attachment 58215 [details]
proposed patch to mark children/dynamic objects created through a qobject
[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. Created attachment 58272 [details]
proposed patch to mark children/dynamic objects created through a qobject
Created attachment 58554 [details]
Proposed patch with proper coding style
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. (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. Created attachment 59429 [details]
Rediff against trunk
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.
In general however I think this patch looks good (I forgot to mention that earlier) Created attachment 59738 [details]
Patch
(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. 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.
Committed r62898: <http://trac.webkit.org/changeset/62898> This change <http://trac.webkit.org/changeset/62898> causes tst_QWebFrame to crash. Reopening. I opened bug 43039 for the regression introduced by this change. |