171602 2017-05-03 05:36:42 -0700 REGRESSION(r215686): Incremental reads from SharedBuffer are wrong after r215686 2017-05-08 09:03:13 -0700 1 1 1 Unclassified WebKit Platform WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=170956 Gtk, LayoutTestFailure P2 Normal --- 1 cgarcia webkit-unassigned achristensen ap buildbot cdumez dbates eric.carlson japhet jer.noble mcatanzaro oldest_to_newest 1304008 0 cgarcia 2017-05-03 05:36:42 -0700 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 1304010 1 308899 cgarcia 2017-05-03 05:42:33 -0700 Created attachment 308899 Patch 1304392 2 308899 ap 2017-05-03 16:59:41 -0700 Comment on attachment 308899 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? 1304524 3 cgarcia 2017-05-03 23:00:39 -0700 (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. 1304529 4 cgarcia 2017-05-03 23:14:46 -0700 Committed r216174: <http://trac.webkit.org/changeset/216174> 1305871 5 308899 achristensen 2017-05-08 09:03:13 -0700 Comment on attachment 308899 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. 308899 2017-05-03 05:42:33 -0700 2017-05-03 09:47:11 -0700 Patch wk-shared-buffer-partial-read.diff text/plain 3137 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3ODIzZjhkNzQwYi4uMzljYzhjNjdmMDggMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMiBAQAorMjAxNy0wNS0wMyAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGln YWxpYS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTihyMjE1Njg2KTogSW5jcmVtZW50YWwgcmVh ZHMgZnJvbSBTaGFyZWRCdWZmZXIgYXJlIHdyb25nIGFmdGVyIHIyMTU2ODYKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3MTYwMgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEluIFRleHRUcmFja0xvYWRlcjo6 cHJvY2Vzc05ld0N1ZURhdGEoKSBhbmQgUE5HSW1hZ2VSZWFkZXI6OmRlY29kZSgpIHdlIGNoYW5n ZWQgdGhlIHBhdHRlciB0byByZWFkIGRhdGEgZnJvbSBhCisgICAgICAgIFNoYXJlZEJ1ZmZlciBh dCBhIGdpdmVuIG9mZnNldC4gVGhlIG5ldyBwYXR0ZXJuIGlzIG5vdCBjb3JyZWN0LCBiZWNhdXNl IGl0IGFzc3VtZXMgdGhlIHdob2xlIHNlZ21lbnQgaXMgYWx3YXlzCisgICAgICAgIHJlYWQsIGFu ZCB0aGUgbmV3IG9mZnNldCBpcyBub3QgY29ycmVjdCB3aGVuIHRoYXQncyBub3QgdGhlIGNhc2Uu IFRoaXMgaGFzIGJyb2tlbiB0aGUgcmVuZGVyaW5nIG9mIHBuZyBpbWFnZXMgaW4KKyAgICAgICAg dGhlIEdUSysgcG9ydCwgb25seSB0aGUgZmlyc3QgYnl0ZXMgYXJlIGNvcnJlY3RseSBkZWNvZGVk IGFuZCBkcmF3biwgYnV0IG5vdCB0aGUgcmVzdCBvZiB0aGUgaW1hZ2UuCisKKyAgICAgICAgRml4 ZXM6IGVkaXRpbmcvcGFzdGVib2FyZC9wYXN0ZS1pbWFnZS11c2luZy1pbWFnZS1kYXRhLmh0bWwK KworICAgICAgICAqIGxvYWRlci9UZXh0VHJhY2tMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6VGV4dFRyYWNrTG9hZGVyOjpwcm9jZXNzTmV3Q3VlRGF0YSk6CisgICAgICAgICogcGxhdGZv cm0vaW1hZ2UtZGVjb2RlcnMvcG5nL1BOR0ltYWdlRGVjb2Rlci5jcHA6CisgICAgICAgIChXZWJD b3JlOjpQTkdJbWFnZVJlYWRlcjo6ZGVjb2RlKToKKwogMjAxNy0wNS0wMiAgQW5kcmV3IEdvbGQg IDxhZ29sZEBhcHBsZS5jb20+CiAKICAgICAgICAgVHlwbyBpbiBBVkF1ZGlvU2Vzc2lvbkNhcHR1 cmVEZXZpY2VNYW5hZ2VyLm1tCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvVGV4 dFRyYWNrTG9hZGVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9UZXh0VHJhY2tMb2FkZXIu Y3BwCmluZGV4IDcyOGZlMzM1MTMxLi4yMjE1N2U5YjIyMyAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvbG9hZGVyL1RleHRUcmFja0xvYWRlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvbG9h ZGVyL1RleHRUcmFja0xvYWRlci5jcHAKQEAgLTk3LDkgKzk3LDEwIEBAIHZvaWQgVGV4dFRyYWNr TG9hZGVyOjpwcm9jZXNzTmV3Q3VlRGF0YShDYWNoZWRSZXNvdXJjZSYgcmVzb3VyY2UpCiAgICAg ICAgICAgICBieXRlc1RvU2tpcCAtPSBzZWdtZW50LT5zaXplKCk7CiAgICAgICAgICAgICBjb250 aW51ZTsKICAgICAgICAgfQotICAgICAgICBtX2N1ZVBhcnNlci0+cGFyc2VCeXRlcyhzZWdtZW50 LT5kYXRhKCkgKyBieXRlc1RvU2tpcCwgc2VnbWVudC0+c2l6ZSgpIC0gYnl0ZXNUb1NraXApOwor ICAgICAgICBhdXRvIGJ5dGVzVG9Vc2UgPSBzZWdtZW50LT5zaXplKCkgLSBieXRlc1RvU2tpcDsK KyAgICAgICAgbV9jdWVQYXJzZXItPnBhcnNlQnl0ZXMoc2VnbWVudC0+ZGF0YSgpICsgYnl0ZXNU b1NraXAsIGJ5dGVzVG9Vc2UpOwogICAgICAgICBieXRlc1RvU2tpcCA9IDA7Ci0gICAgICAgIG1f cGFyc2VPZmZzZXQgKz0gc2VnbWVudC0+c2l6ZSgpOworICAgICAgICBtX3BhcnNlT2Zmc2V0ICs9 IGJ5dGVzVG9Vc2U7CiAgICAgfQogfQogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9pbWFnZS1kZWNvZGVycy9wbmcvUE5HSW1hZ2VEZWNvZGVyLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL3BuZy9QTkdJbWFnZURlY29kZXIuY3BwCmluZGV4 IGNmYzhiZjkxZjE3Li4xN2M1NzA3NjZlZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vaW1hZ2UtZGVjb2RlcnMvcG5nL1BOR0ltYWdlRGVjb2Rlci5jcHAKKysrIGIvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vaW1hZ2UtZGVjb2RlcnMvcG5nL1BOR0ltYWdlRGVjb2Rlci5jcHAK QEAgLTE2Myw5ICsxNjMsMTAgQEAgcHVibGljOgogICAgICAgICAgICAgICAgIGJ5dGVzVG9Ta2lw IC09IHNlZ21lbnQtPnNpemUoKTsKICAgICAgICAgICAgICAgICBjb250aW51ZTsKICAgICAgICAg ICAgIH0KLSAgICAgICAgICAgIG1fcmVhZE9mZnNldCArPSBzZWdtZW50LT5zaXplKCk7CisgICAg ICAgICAgICBhdXRvIGJ5dGVzVG9Vc2UgPSBzZWdtZW50LT5zaXplKCkgLSBieXRlc1RvU2tpcDsK KyAgICAgICAgICAgIG1fcmVhZE9mZnNldCArPSBieXRlc1RvVXNlOwogICAgICAgICAgICAgbV9j dXJyZW50QnVmZmVyU2l6ZSA9IG1fcmVhZE9mZnNldDsKLSAgICAgICAgICAgIHBuZ19wcm9jZXNz X2RhdGEobV9wbmcsIG1faW5mbywgcmVpbnRlcnByZXRfY2FzdDxwbmdfYnl0ZXA+KGNvbnN0X2Nh c3Q8Y2hhcio+KHNlZ21lbnQtPmRhdGEoKSArIGJ5dGVzVG9Ta2lwKSksIHNlZ21lbnQtPnNpemUo KSAtIGJ5dGVzVG9Ta2lwKTsKKyAgICAgICAgICAgIHBuZ19wcm9jZXNzX2RhdGEobV9wbmcsIG1f aW5mbywgcmVpbnRlcnByZXRfY2FzdDxwbmdfYnl0ZXA+KGNvbnN0X2Nhc3Q8Y2hhcio+KHNlZ21l bnQtPmRhdGEoKSArIGJ5dGVzVG9Ta2lwKSksIGJ5dGVzVG9Vc2UpOwogICAgICAgICAgICAgYnl0 ZXNUb1NraXAgPSAwOwogICAgICAgICAgICAgLy8gV2UgZXhwbGljaXRseSBzcGVjaWZ5IHRoZSBz dXBlcmNsYXNzIGVuY29kZWREYXRhU3RhdHVzKCkgYmVjYXVzZSB3ZQogICAgICAgICAgICAgLy8g bWVyZWx5IHdhbnQgdG8gY2hlY2sgaWYgd2UndmUgbWFuYWdlZCB0byBzZXQgdGhlIHNpemUsIG5v dAo=