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+

Description Myles C. Maxfield 2014-03-20 15:16:54 PDT
InlineIterator position can wrap around
Comment 1 Myles C. Maxfield 2014-03-20 15:38:37 PDT
Created attachment 227347 [details]
Patch
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Dave Hyatt 2014-03-21 14:30:18 PDT
Comment on attachment 227347 [details]
Patch

r=me
Comment 5 Myles C. Maxfield 2014-03-21 15:06:20 PDT
Unable to reproduce test failures.
Comment 6 Myles C. Maxfield 2014-03-21 15:10:01 PDT
Created attachment 227497 [details]
Resubmit to EWS
Comment 7 Darin Adler 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?
Comment 8 Myles C. Maxfield 2014-03-24 20:44:33 PDT
Created attachment 227718 [details]
Patch
Comment 9 Myles C. Maxfield 2014-03-24 21:39:41 PDT
Created attachment 227719 [details]
More asserts
Comment 10 Simon Fraser (smfr) 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.
Comment 11 Myles C. Maxfield 2014-03-24 23:38:48 PDT
Can you explain what you mean by "wraparound amount?"
Comment 12 Myles C. Maxfield 2014-03-25 01:13:36 PDT
Created attachment 227728 [details]
InlineIterator has new boolean value
Comment 13 Simon Fraser (smfr) 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.
Comment 14 Simon Fraser (smfr) 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.
Comment 15 Myles C. Maxfield 2014-03-25 12:48:03 PDT
*** Bug 130552 has been marked as a duplicate of this bug. ***
Comment 16 Myles C. Maxfield 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.
Comment 17 Myles C. Maxfield 2014-03-25 13:16:15 PDT
https://trac.webkit.org/changeset/166245
Comment 18 Simon Fraser (smfr) 2014-03-26 19:41:15 PDT
This caused one test to assert:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r166328%20(3608)/results.html
Comment 19 Myles C. Maxfield 2014-03-27 11:52:54 PDT
Failing test
Comment 20 Myles C. Maxfield 2014-03-27 12:10:37 PDT
<rdar://problem/16432098>
<rdar://problem/15215021>
Comment 21 Myles C. Maxfield 2014-03-31 14:51:12 PDT
Fixed in https://bugs.webkit.org/show_bug.cgi?id=130874