Bug 45095

Summary:

absolutely positioned list in inline element crashes

Product:

WebKit

Reporter:

Shinichiro Hamaji <hamaji>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, mitz, yuzo

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch v1	none
Patch v2 (minor comment change)	none
Patch only for NULL crash	none

Shinichiro Hamaji

Reported 2010-09-02 02:07:40 PDT

WebKit crashes for <span> <li> <li style="position: absolute; list-style-type: upper-roman;"> The render tree for this HTML is layer at (0,0) size 800x600 RenderView at (0,0) size 800x600 layer at (0,0) size 800x600 RenderBlock {HTML} at (0,0) size 800x600 RenderBody {BODY} at (8,8) size 784x584 RenderBlock (anonymous) at (0,0) size 784x0 RenderInline {SPAN} at (0,0) size 0x0 RenderText {#text} at (0,0) size 0x0 RenderBlock (anonymous) at (0,0) size 784x18 RenderListItem {LI} at (0,0) size 784x18 RenderListMarker at (-1,0) size 7x18: bullet RenderBlock (anonymous) at (0,18) size 784x0 RenderInline {SPAN} at (0,0) size 0x0 layer at (8,26) size 18x18 RenderListItem {LI} at (8,26) size 18x18 RenderListMarker at (0,0) size 18x18: "II" This crash happens because enclosingList in RenderListItem.cpp returns different nodes for the two <li> elements. Using only block element in enclosingList would fix this issue, I'm not 100% sure if this is the best fix though. A patch will come soon later. (CCing mitz as he seemed to touch this code recently in http://trac.webkit.org/changeset/53868)

Attachments
Patch v1 (3.69 KB, patch) 2010-09-02 02:09 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch v2 (minor comment change) (3.82 KB, patch) 2010-09-02 08:29 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch only for NULL crash (3.46 KB, patch) 2010-10-14 00:24 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Shinichiro Hamaji

Comment 1 2010-09-02 02:09:35 PDT

Created attachment 66340 [details] Patch v1

mitz

Comment 2 2010-09-02 02:25:41 PDT

Comment on attachment 66340 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=66340&action=prettypatch > WebCore/rendering/RenderListItem.cpp:100 > // If there's no actual <ul> or <ol> list element, then the first found > // node acts as our list for purposes of determining what other list items > // should be numbered as part of the same list. > - return firstNode; > + return firstBlockNode; The comment says the the first found node is used, but you changed it to be the first found node that is rendered as a block. Why is this the right thing to do? What if the document element itself is not a RenderBlock? Can this function return 0?

Shinichiro Hamaji

Comment 3 2010-09-02 08:29:36 PDT

Created attachment 66373 [details] Patch v2 (minor comment change)

Shinichiro Hamaji

Comment 4 2010-09-02 08:29:55 PDT

Thanks for your review! > The comment says the the first found node is used, but you changed it to be the first found node that is rendered as a block. Why is this the right thing to do? What if the document element itself is not a RenderBlock? Can this function return 0? I think we still have <html> as the last block element? Can this be missing or inline? I think I've never seen the case where the HTML element is inline, but of course I may be wrong. If I'm wrong, Could you tell me how I can make the root element inline? I've updated the comment for now. By the way, I don't really know what we should do when there are no <ul> and <ol>. I guess this case is not specified well. FYI, Firefox seems to output "0", not "II" for the test case. If we should prefer Firefox's behavior, I'll update my patch. I like the current WebKit's behavior though. Thanks!

mitz

Comment 5 2010-09-02 09:12:23 PDT

(In reply to comment #4) > Thanks for your review! > > > The comment says the the first found node is used, but you changed it to be the first found node that is rendered as a block. Why is this the right thing to do? What if the document element itself is not a RenderBlock? Can this function return 0? > > I think we still have <html> as the last block element? Can this be missing or inline? I think I've never seen the case where the HTML element is inline, but of course I may be wrong. If I'm wrong, Could you tell me how I can make the root element inline? I've updated the comment for now. I was wrong. The document element is indeed forced to be a block. However, according to the comment in <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderView.cpp#L211> this may not always be the case. > By the way, I don't really know what we should do when there are no <ul> and <ol>. I guess this case is not specified well. FYI, Firefox seems to output "0", not "II" for the test case. If we should prefer Firefox's behavior, I'll update my patch. I like the current WebKit's behavior though. > > Thanks! Can you explain the significance of returning a node that is rendered as a block?

Shinichiro Hamaji

Comment 6 2010-09-03 07:49:26 PDT

> Can you explain the significance of returning a node that is rendered as a block? In the test case, the first <li> considers <body> is its enclosing node and the second <li>'s enclosing node is <span>. If we ignore <span>, the second <li> also consider's <body> as its enclosing node, so the two <li>s have the same enclosing node and previousListItem returns <body> before it touches a NULL renderer. Also, given <ul> and <ol> are blocks, I couldn't imagine the case where having always block enclosing nodes, though it's not good if the document element can be inline. Anyway, as I wrote, I'm not sure if this fix is the best. Perhaps we need to change the position of the continued <span>, but I'm not sure if it's easy to fix this bug this way. The current render tree is like RenderView 0x11477fc #document 0x886f000 RenderBlock 0x11af77c HTML 0x1161150 RenderBody 0x114302c BODY 0x11a4460 RenderBlock (anonymous) 0x11a8d0c RenderInline 0x11a74bc SPAN 0x11afca0 RenderText 0x11ab47c #text 0x1154e60 "\n" RenderBlock (anonymous) 0x11abccc RenderListItem 0x11afe0c LI 0x11a95d0 RenderBlock (anonymous) 0x11a8b8c RenderInline 0x11a85ec SPAN 0x11afca0 RenderListItem 0x11a845c LI 0x11a87b0 STYLE=position:absolute The above render tree isn't good and it should be like the following? RenderView 0x11477fc #document 0x886f000 RenderBlock 0x11af77c HTML 0x1161150 RenderBody 0x114302c BODY 0x11a4460 RenderBlock (anonymous) 0x11a8d0c RenderInline 0x11a74bc SPAN 0x11afca0 RenderText 0x11ab47c #text 0x1154e60 "\n" RenderBlock (anonymous) 0x11abccc RenderListItem 0x11afe0c LI 0x11a95d0 RenderBlock (anonymous) 0x11a8b8c RenderListItem 0x11a845c LI 0x11a87b0 STYLE=position:absolute RenderBlock (anonymous) 0x11a8bXX RenderInline 0x11a85ec SPAN 0x11afca0

Shinichiro Hamaji

Comment 7 2010-09-30 22:27:17 PDT

Ping? Please let me know if my description is unclear. Thanks,

Yuzo Fujishima

Comment 8 2010-10-06 17:08:42 PDT

Ping? (on behalf of Shinichiro)

Shinichiro Hamaji

Comment 9 2010-10-14 00:24:27 PDT

Created attachment 70714 [details] Patch only for NULL crash

Shinichiro Hamaji

Comment 10 2010-10-14 00:26:21 PDT

OK, I guess Dan is busy these days. So, as a crash bug is somewhat serious, let's fix the crash first then try fixing wrong numbering later. The patch I've just submitted has a failing layout test, but at least it doesn't crash. If this patch is accepted, I'll file another bug for the failing test.

Kent Tamura

Comment 11 2010-10-19 00:35:01 PDT

Comment on attachment 70714 [details] Patch only for NULL crash Looks ok.

WebKit Commit Bot

Comment 12 2010-10-20 00:41:31 PDT

The commit-queue encountered the following flaky tests while processing attachment 70714 [details]: http/tests/security/xssAuditor/script-tag-with-source-double-quote.html Please file bugs against the tests. The author(s) of the test(s) have been CCed on this bug. The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 13 2010-10-20 00:44:57 PDT

Comment on attachment 70714 [details] Patch only for NULL crash Clearing flags on attachment: 70714 Committed r70132: <http://trac.webkit.org/changeset/70132>

WebKit Commit Bot

Comment 14 2010-10-20 00:45:03 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.