Bug 60802 - Make GC validation more aggressive
Summary: Make GC validation more aggressive
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on: 60881
Blocks:
  Show dependency tree / graph
 
Reported: 2011-05-13 14:11 PDT by Oliver Hunt
Modified: 2011-05-16 05:23 PDT (History)
6 users (show)

See Also:

Attachments
Patch (75.16 KB, patch)
2011-05-13 14:17 PDT, Oliver Hunt 		no flags Details | Formatted Diff | Diff
Patch (75.25 KB, patch)
2011-05-13 15:31 PDT, Oliver Hunt 		no flags Details | Formatted Diff | Diff
crash log from debug Qt bot (3.94 KB, text/plain)
2011-05-14 01:09 PDT, Csaba Osztrogonác 		no flags Details
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-05-13 14:11:49 PDT 
Make GC validation more aggressive
Comment 1 Oliver Hunt 2011-05-13 14:17:28 PDT 
Created attachment 93509 [details]
Patch
Comment 2 Collabora GTK+ EWS bot 2011-05-13 14:29:47 PDT 
Comment on attachment 93509 [details]
Patch

Attachment 93509 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8698179
Comment 3 Early Warning System Bot 2011-05-13 14:33:27 PDT 
Comment on attachment 93509 [details]
Patch

Attachment 93509 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8695239
Comment 4 Oliver Hunt 2011-05-13 15:31:53 PDT 
Created attachment 93524 [details]
Patch
Comment 5 Geoffrey Garen 2011-05-13 15:42:05 PDT 
Comment on attachment 93524 [details]
Patch

r=me, assuming everything builds and doesn't crash
Comment 6 Oliver Hunt 2011-05-13 16:07:37 PDT 
Committed r86469: <http://trac.webkit.org/changeset/86469>
Comment 7 Csaba Osztrogonác 2011-05-14 01:05:43 PDT 
(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.
Comment 8 Csaba Osztrogonác 2011-05-14 01:09:00 PDT 
Created attachment 93550 [details]
crash log from debug Qt bot
Comment 9 Oliver Hunt 2011-05-14 10:28:25 PDT 
(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.
Comment 10 Oliver Hunt 2011-05-14 10:30:42 PDT 
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.
Comment 11 Oliver Hunt 2011-05-14 10:35:59 PDT 
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.
Comment 12 Csaba Osztrogonác 2011-05-14 12:54:13 PDT 
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.
Comment 13 Oliver Hunt 2011-05-14 12:57:16 PDT 
(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.
Comment 14 Oliver Hunt 2011-05-14 15:10:18 PDT 
Committed r86499: <http://trac.webkit.org/changeset/86499>