Bug 48979

Summary: [Chromium] SVGListPropertyTearOff.h: function commitChange ASSERTs on Win & Mac
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mdelaney7, zimmermann
Priority: P2 Keywords: LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 47905    
Attachments:
Description Flags
What Nikolas wrote none

Description Roland Steiner 2010-11-03 23:47:22 PDT
The function commitChange() in SVGListPropertyTearOff.h started to run into " ASSERT(size == values.size()); " on Chromium Windows and Mac (at least), sometimes between WK r71224 and r71305.

The failed condition seems to have no adverse effect in Release (where the ASSERT isn't run), but it still seems fishy:

'values' has 1 entry, while 'wrappers' has 2, with the first entry being NULL.
Comment 1 Nikolas Zimmermann 2010-11-04 00:57:11 PDT
I'll investigate today.
Comment 2 Nikolas Zimmermann 2010-11-04 09:02:01 PDT
Interessting, the garbage collection in v8 is freeing the SVGPropertyTearOff wrapper earlier than JSC, and thanks to that I found the bug:


    PassListItemTearOff removeItemValuesAndWrappers(AnimatedListPropertyTearOff* animatedList, unsigned index, ExceptionCode& ec)
    {
....
        // Detach the existing wrapper.
        RefPtr<ListItemTearOff>& oldItem = wrappers.at(index);
        if (oldItem) {
            oldItem->detachWrapper();
            wrappers.remove(index);
        }


The wrappers.remove(index) needs to be moved out of the if clause, otherwhise the list sizes don't match. Unfortunately I need to leave now :(
I think I can fix it tomorrow or tonight.
Or if anyone else wants to do that, here's how to fix :-)
Comment 3 Roland Steiner 2010-11-04 22:24:57 PDT
Created attachment 73035 [details]
What Nikolas wrote
Comment 4 Dirk Schulze 2010-11-04 23:31:40 PDT
Comment on attachment 73035 [details]
What Nikolas wrote

LGTM. r=me
Comment 5 Nikolas Zimmermann 2010-11-05 02:08:54 PDT
Comment on attachment 73035 [details]
What Nikolas wrote

Setting cq+ to get this in ASAP, as my SVGPointList patch also hits the assertion w/o the patch and it's soon ready to be reviewed.
Comment 6 WebKit Commit Bot 2010-11-05 02:41:40 PDT
The commit-queue encountered the following flaky tests while processing attachment 73035 [details]:

http/tests/appcache/deferred-events-delete-while-raising.html

Please file bugs against the tests.  These tests were authored by michaeln@google.com.  The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2010-11-05 02:42:57 PDT
Comment on attachment 73035 [details]
What Nikolas wrote

Clearing flags on attachment: 73035

Committed r71399: <http://trac.webkit.org/changeset/71399>
Comment 8 WebKit Commit Bot 2010-11-05 02:43:03 PDT
All reviewed patches have been landed.  Closing bug.