Bug 48979 - [Chromium] SVGListPropertyTearOff.h: function commitChange ASSERTs on Win & Mac
Summary: [Chromium] SVGListPropertyTearOff.h: function commitChange ASSERTs on Win & Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: LayoutTestFailure
Depends on:
Blocks: 47905
  Show dependency treegraph
 
Reported: 2010-11-03 23:47 PDT by Roland Steiner
Modified: 2010-11-05 02:43 PDT (History)
3 users (show)

See Also:


Attachments
What Nikolas wrote (2.96 KB, patch)
2010-11-04 22:24 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.