| 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: | P1 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 66340 [details]
Patch v1
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? Created attachment 66373 [details]
Patch v2 (minor comment change)
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!
(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? > 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
Ping? Please let me know if my description is unclear. Thanks, Ping? (on behalf of Shinichiro) Created attachment 70714 [details]
Patch only for NULL crash
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. Comment on attachment 70714 [details]
Patch only for NULL crash
Looks ok.
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.
Comment on attachment 70714 [details] Patch only for NULL crash Clearing flags on attachment: 70714 Committed r70132: <http://trac.webkit.org/changeset/70132> All reviewed patches have been landed. Closing bug. |
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)