Bug 171602

Summary: REGRESSION(r215686): Incremental reads from SharedBuffer are wrong after r215686
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, buildbot, cdumez, dbates, eric.carlson, japhet, jer.noble, mcatanzaro
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=170956
Attachments:
Description Flags
Patch mcatanzaro: review+

Carlos Garcia Campos
Reported 2017-05-03 05:36:42 PDT
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
Attachments
Patch (3.06 KB, patch)
2017-05-03 05:42 PDT, Carlos Garcia Campos
mcatanzaro: review+
Carlos Garcia Campos
Comment 1 2017-05-03 05:42:33 PDT
Alexey Proskuryakov
Comment 2 2017-05-03 16:59:41 PDT
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
Comment 3 2017-05-03 23:00:39 PDT
(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
Comment 4 2017-05-03 23:14:46 PDT
Alex Christensen
Comment 5 2017-05-08 09:03:13 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.