Bug 46886 - [GTK] Crash on testatk.c unittest because of a call to ASSERT_NOT_REACHED
Summary: [GTK] Crash on testatk.c unittest because of a call to ASSERT_NOT_REACHED
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-30 04:43 PDT by Mario Sanchez Prada
Modified: 2010-09-30 18:24 PDT (History)
4 users (show)

See Also:


Attachments
Patch proposal (3.36 KB, patch)
2010-09-30 04:57 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2010-09-30 04:43:59 PDT
It looks that the following crash has been happening in the Debug build of the GTK port since the patch for bug 45381 was applied (see http://trac.webkit.org/changeset/68415):

Program terminated with signal 11, Segmentation fault.
#0  0x55fb8d13 in listMarkerSuffix (type=WebCore::Disc, value=1)
    at ../../WebCore/rendering/RenderListMarker.cpp:507
507	        ASSERT_NOT_REACHED();

This is caused due to the new function WebCore::RenderListMarker::suffix() added along with that patch, which uses the internal function listMarkerSuffix() to retrieve the marker's suffix and return its value.

Adding Daniel Bates to CC as its name is in commit revision 53452 (bug 33089), where the original code was introduced.
Comment 1 Mario Sanchez Prada 2010-09-30 04:57:08 PDT
Created attachment 69325 [details]
Patch proposal

Attaching a patch proposal that just gets rid of the ASSERT_NOT_REACHED() statement, as it's my understanding that's no longer correct. As explained in the ChangeLog entry:

        So far, this assertion made sense because it was not possible that
        a call to listMarkerSuffix() happened when the style of the list
        item was one of the following: Asterisks, Circle, Discm Footnotes,
        NoneListStyle or Square (it's easy to figure this out by checking
        the functions where listMarkerSuffix() was called).

        However, since revision 68415 (about bug 45381), there's a new
        place where listMarkerSuffix() is being called (the suffix()
        public method), regardless of the style of the item being or not
        one of those pointed out, so the aforementioned assertion in
        listMarkerSuffix() would no longer be correct, as now it's ok to
        reach those cases in the switch statement.


Daniel, would you mind taking a look to this. I guess you're probably one of the best persons to say whether this approach is correct or not.

Thanks in advance
Comment 2 Daniel Bates 2010-09-30 16:35:50 PDT
Comment on attachment 69325 [details]
Patch proposal

This seems reasonable to me.
Comment 3 Martin Robinson 2010-09-30 16:37:46 PDT
Comment on attachment 69325 [details]
Patch proposal

Setting the cq bit, since Mario is still waiting on his committer access.
Comment 4 WebKit Commit Bot 2010-09-30 18:24:52 PDT
Comment on attachment 69325 [details]
Patch proposal

Clearing flags on attachment: 69325

Committed r68858: <http://trac.webkit.org/changeset/68858>
Comment 5 WebKit Commit Bot 2010-09-30 18:24:57 PDT
All reviewed patches have been landed.  Closing bug.