Bug 171602 – REGRESSION(r215686): Incremental reads from SharedBuffer are wrong after r215686

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

In TextTrackLoader::processNewCueData() and PNGImageReader::decode() we changed the patter to read data from a SharedBuffer at a given offset. The new pattern is not correct, because it assumes the whole segment is always read, and the new offset is not correct when that's not the case. This has broken the rendering of png images in the GTK+ port, only the first bytes are correctly decoded and drawn, but not the rest of the image. See for example:

https://build.webkit.org/results/GTK Linux 64-bit Release (Tests)/r216114 (888)/editing/pasteboard/paste-image-using-image-data-diff.png

[Carlos Garcia Campos]

Created attachment 308899 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 308899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308899&action=review

> Source/WebCore/loader/TextTrackLoader.cpp:103
> -        m_cueParser->parseBytes(segment->data() + bytesToSkip, segment->size() - bytesToSkip);
> +        auto bytesToUse = segment->size() - bytesToSkip;
> +        m_cueParser->parseBytes(segment->data() + bytesToSkip, bytesToUse);
>          bytesToSkip = 0;
> -        m_parseOffset += segment->size();
> +        m_parseOffset += bytesToUse;

There doesn't seem to be a test for this part of the patch. What does this fix?

[Carlos Garcia Campos]

(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 308899 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308899&action=review
> 
> > Source/WebCore/loader/TextTrackLoader.cpp:103
> > -        m_cueParser->parseBytes(segment->data() + bytesToSkip, segment->size() - bytesToSkip);
> > +        auto bytesToUse = segment->size() - bytesToSkip;
> > +        m_cueParser->parseBytes(segment->data() + bytesToSkip, bytesToUse);
> >          bytesToSkip = 0;
> > -        m_parseOffset += segment->size();
> > +        m_parseOffset += bytesToUse;
> 
> There doesn't seem to be a test for this part of the patch. What does this
> fix?

I don't even know what TextTrackLoader is, but the pattern to read the data is exactly the same as in PNGEncoder, so this will fail the same way in case of incremental reads where bytesToSkip != 0 in any of the reads. Alex said he was going to add API to SharedBuffer to make it easier to read this way, so this and PNGEncoder will be updated too eventually.

[Carlos Garcia Campos]

Committed r216174: <http://trac.webkit.org/changeset/216174>

[Alex Christensen]

Comment on attachment 308899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308899&action=review

>>> Source/WebCore/loader/TextTrackLoader.cpp:103
>>> +        m_parseOffset += bytesToUse;
>> 
>> There doesn't seem to be a test for this part of the patch. What does this fix?
> 
> I don't even know what TextTrackLoader is, but the pattern to read the data is exactly the same as in PNGEncoder, so this will fail the same way in case of incremental reads where bytesToSkip != 0 in any of the reads. Alex said he was going to add API to SharedBuffer to make it easier to read this way, so this and PNGEncoder will be updated too eventually.

To clarify, we shouldn't add API to SharedBuffer to read from a certain offset.  Since we always read entire segments from SharedBuffers, we should stop keeping track of how many bytes we have read and instead keep track of how many segments we have read.  Then we can skip to a segment with a known index instead of iterating all the segments and counting the number of bytes to skip.  This will require new SharedBuffer API, and it will make loading actually make sense.