Bug 60802 – Make GC validation more aggressive

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Make GC validation more aggressive

[Oliver Hunt]

Created attachment 93509 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 93509 [details]
Patch

Attachment 93509 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8698179

[Early Warning System Bot]

Comment on attachment 93509 [details]
Patch

Attachment 93509 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8695239

[Oliver Hunt]

Created attachment 93524 [details]
Patch

[Geoffrey Garen]

Comment on attachment 93524 [details]
Patch

r=me, assuming everything builds and doesn't crash

[Oliver Hunt]

Committed r86469: <http://trac.webkit.org/changeset/86469>

[Csaba Osztrogonác]

(In reply to comment #5)
> (From update of attachment 93524 [details])
> r=me, assuming everything builds and doesn't crash

(In reply to comment #6)
> Committed r86469: <http://trac.webkit.org/changeset/86469>

Unfortunately you didn't take Geoff's advice and caused hundreds crashes on Qt bot. Because of this I had to roll out: http://trac.webkit.org/changeset/86482

Reopen to fix it. I'll attach a crash log to help debugging.

[Csaba Osztrogonác]

Created attachment 93550 [details]
crash log from debug Qt bot

[Oliver Hunt]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 93524 [details] [details])
> > r=me, assuming everything builds and doesn't crash
> 
> (In reply to comment #6)
> > Committed r86469: <http://trac.webkit.org/changeset/86469>
> 
> Unfortunately you didn't take Geoff's advice and caused hundreds crashes on Qt bot. Because of this I had to roll out: http://trac.webkit.org/changeset/86482

Thanks for the snarky comment.

> 
> Reopen to fix it. I'll attach a crash log to help debugging.
Crash log doesn't help because it's stripped of real symbol information.  We've had this problem with Qt before.  The build bots cannot strip symbols.

[Oliver Hunt]

I'll be relanding this shortly on the assumption that the qt bot failures are due to errors in qt code rather than errors in the patch itself.  Given this patch had one purpose (find errors in code using jsc) the response to many errors should have been to determine what the errors were, not roll out the patch that identified them.

[Oliver Hunt]

Lo and behold it _is_ an error in qt.

~QtRuntimeMethod removes itself from the QtInstance method cache during it's destructor, which is a point at which it is no longer guaranteed to be a valid object.

QtRuntimeMethod should use a finalizer to remove itself from the map not its destructor.

[Csaba Osztrogonác]

Thanks for the additional information. Please don't land it at the 
weekend, zherczeg will gladly fix the bug in Qt code path at monday 
morning (in european timezone).

Thanks for your patience.

[Oliver Hunt]

(In reply to comment #12)
> Thanks for the additional information. Please don't land it at the 
> weekend, zherczeg will gladly fix the bug in Qt code path at monday 
> morning (in european timezone).
> 
> Thanks for your patience.

We actually need this patch in as we're seeing crashes due to gc bugs that this will expose.  Happily I've put up a patch at https://bugs.webkit.org/show_bug.cgi?id=60841 that should fix the issue.  Currently waiting on the EWS to tell me how to make it build properly.

[Oliver Hunt]

Committed r86499: <http://trac.webkit.org/changeset/86499>