Bug 130540

Summary: InlineIterator position (unsigned int) variable can wrap around
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dino, esprehn+autocc, glenn, hyatt, kondapallykalyan, rniwa, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Resubmit to EWS
none
Patch
none
More asserts
none
InlineIterator has new boolean value simon.fraser: review+

Myles C. Maxfield
Reported 2014-03-20 15:16:54 PDT
InlineIterator position can wrap around
Attachments
Patch (3.72 KB, patch)
2014-03-20 15:38 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (686.72 KB, application/zip)
2014-03-20 17:43 PDT, Build Bot
no flags
Resubmit to EWS (3.71 KB, patch)
2014-03-21 15:10 PDT, Myles C. Maxfield
no flags
Patch (11.57 KB, patch)
2014-03-24 20:44 PDT, Myles C. Maxfield
no flags
More asserts (11.88 KB, patch)
2014-03-24 21:39 PDT, Myles C. Maxfield
no flags
InlineIterator has new boolean value (13.82 KB, patch)
2014-03-25 01:13 PDT, Myles C. Maxfield
simon.fraser: review+
Myles C. Maxfield
Comment 1 2014-03-20 15:38:37 PDT
Build Bot
Comment 2 2014-03-20 17:43:02 PDT
Comment on attachment 227347 [details] Patch Attachment 227347 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5871879202013184 New failing tests: media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html
Build Bot
Comment 3 2014-03-20 17:43:06 PDT
Created attachment 227358 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Dave Hyatt
Comment 4 2014-03-21 14:30:18 PDT
Comment on attachment 227347 [details] Patch r=me
Myles C. Maxfield
Comment 5 2014-03-21 15:06:20 PDT
Unable to reproduce test failures.
Myles C. Maxfield
Comment 6 2014-03-21 15:10:01 PDT
Created attachment 227497 [details] Resubmit to EWS
Darin Adler
Comment 7 2014-03-22 08:08:45 PDT
Comment on attachment 227497 [details] Resubmit to EWS View in context: https://bugs.webkit.org/attachment.cgi?id=227497&action=review > LayoutTests/ChangeLog:9 > + * fast/text/whitespace-only-text-in-rtl-expected.txt: Added. > + * fast/text/whitespace-only-text-in-rtl.html: Added. It’s non-obvious what this is testing, and what passing or failing is like. Could you change the test so it’s self documenting?
Myles C. Maxfield
Comment 8 2014-03-24 20:44:33 PDT
Myles C. Maxfield
Comment 9 2014-03-24 21:39:41 PDT
Created attachment 227719 [details] More asserts
Simon Fraser (smfr)
Comment 10 2014-03-24 23:02:19 PDT
Comment on attachment 227719 [details] More asserts View in context: https://bugs.webkit.org/attachment.cgi?id=227719&action=review > Source/WebCore/rendering/InlineIterator.h:102 > + // Leave space after this value so we have room to detect wraparound > + static const unsigned kSkipEntireElementOffset = UINT_MAX - 15; I still think this approach is a bit too magical. Why not give this class another bit of state: a bool that's true if we're in the wraparound state, or give it a separate "wraparound amount" if it can be > 1.
Myles C. Maxfield
Comment 11 2014-03-24 23:38:48 PDT
Can you explain what you mean by "wraparound amount?"
Myles C. Maxfield
Comment 12 2014-03-25 01:13:36 PDT
Created attachment 227728 [details] InlineIterator has new boolean value
Simon Fraser (smfr)
Comment 13 2014-03-25 09:11:49 PDT
(In reply to comment #11) > Can you explain what you mean by "wraparound amount?" I think it was always just 1, right? Not sure why you did the -15 in the previous patch though.
Simon Fraser (smfr)
Comment 14 2014-03-25 09:13:48 PDT
Comment on attachment 227728 [details] InlineIterator has new boolean value View in context: https://bugs.webkit.org/attachment.cgi?id=227728&action=review Much more understandable! > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1072 > + unsigned endpointOffset = endpoint.offset(); > + if (endpointOffset) > + endpoint.setOffset(endpointOffset - 1); > + else > + endpoint.setRefersToEndOfPreviousNode(); Seems like this could be a method on InlineIterator.
Myles C. Maxfield
Comment 15 2014-03-25 12:48:03 PDT
*** Bug 130552 has been marked as a duplicate of this bug. ***
Myles C. Maxfield
Comment 16 2014-03-25 12:58:00 PDT
Comment on attachment 227728 [details] InlineIterator has new boolean value View in context: https://bugs.webkit.org/attachment.cgi?id=227728&action=review >> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:1072 >> + endpoint.setRefersToEndOfPreviousNode(); > > Seems like this could be a method on InlineIterator. Done.
Myles C. Maxfield
Comment 17 2014-03-25 13:16:15 PDT
Simon Fraser (smfr)
Comment 18 2014-03-26 19:41:15 PDT
Myles C. Maxfield
Comment 19 2014-03-27 11:52:54 PDT
Failing test
Myles C. Maxfield
Comment 20 2014-03-27 12:10:37 PDT
Myles C. Maxfield
Comment 21 2014-03-31 14:51:12 PDT
Note You need to log in before you can comment on or make changes to this bug.