63313 2011-06-23 22:48:57 -0700 The return value of SharedBuffer::createWithContentsOfFile must have valid m_size. 2011-07-07 10:29:37 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Linux RESOLVED FIXED P2 Normal --- 0 maniagoon webkit-unassigned andersca darin eric skyul webkit.review.bot oldest_to_newest 426736 0 maniagoon 2011-06-23 22:48:57 -0700 SharedBufferPOSIX.cpp - SharedBuffer::m_size has to be set 426738 1 98460 maniagoon 2011-06-23 22:54:03 -0700 Created attachment 98460 Patch 432507 2 98460 eric 2011-07-05 17:57:35 -0700 Comment on attachment 98460 Patch How do we test this? Is there a public function we should call instead of setting m_size directly? 432511 3 98460 eric 2011-07-05 18:01:24 -0700 Comment on attachment 98460 Patch In general this looks fine though. 432512 4 eric 2011-07-05 18:01:50 -0700 How did you detect that this was broken? 432514 5 eric 2011-07-05 18:04:51 -0700 CCing anders who originally created the SharedBuffer class. It's strange to me that m_size is not used by most ports. 432515 6 98460 eric 2011-07-05 18:06:10 -0700 Comment on attachment 98460 Patch Clearing cq+. It seems to me it would be better for this code to read into a Vector and then create the SharedBuffer from the Vector. SharedBuffer doesn't seem to be designed to be read into directly, it's meant to wrap other data storage mechanisms. Since this POSIX implementation doesn't have any "native" data storage, seems we should use Vector. 432516 7 eric 2011-07-05 18:07:39 -0700 We already have a adoptVector(Vector) which gets all this right, including setting m_size. I worry that there are other things that might be missing from this implementation in the future. re-using the adoptVector(Vector) method may be more future-proof. 432620 8 maniagoon 2011-07-05 22:13:39 -0700 (In reply to comment #2) > (From update of attachment 98460 [details]) > How do we test this? Currently, there is no port in upstream that uses this file. So, we cannot test it for now. > Is there a public function we should call instead of setting m_size directly? There is no setter for m_size. 432621 9 maniagoon 2011-07-05 22:14:13 -0700 (In reply to comment #4) > How did you detect that this was broken? When I used the linux port that is mentioned in https://bugs.webkit.org/show_bug.cgi?id=39348#c6, I found this bug. 432622 10 maniagoon 2011-07-05 22:16:13 -0700 (In reply to comment #7) > We already have a adoptVector(Vector) which gets all this right, including setting m_size. I worry that there are other things that might be missing from this implementation in the future. re-using the adoptVector(Vector) method may be more future-proof. Ok. After modifying it, I'll submit new patch. 433363 11 99961 maniagoon 2011-07-07 02:23:56 -0700 Created attachment 99961 Patch 433366 12 maniagoon 2011-07-07 02:42:21 -0700 Changed to use adoptVector(Vector). 433567 13 99961 eric 2011-07-07 10:18:10 -0700 Comment on attachment 99961 Patch Nice! 433580 14 99961 webkit.review.bot 2011-07-07 10:29:32 -0700 Comment on attachment 99961 Patch Clearing flags on attachment: 99961 Committed r90574: <http://trac.webkit.org/changeset/90574> 433581 15 webkit.review.bot 2011-07-07 10:29:37 -0700 All reviewed patches have been landed. Closing bug. 98460 2011-06-23 22:54:03 -0700 2011-07-07 02:23:48 -0700 Patch bug-63313-20110624145355.patch text/plain 1410 maniagoon U3VidmVyc2lvbiBSZXZpc2lvbjogODk2NTAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA2Y2Q4NTZiYjBhZmRhYzc5 ZDVmODljZWVjOWY2MDY4YjUyZDJkZWU3Li43OGYwZWQzZTdkMWRkNmMxNWQ3NzEyZDZhZWEwZWU2 NmQzMjVkNzk3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTEtMDYtMjMgIEt5dXN1 biBLaW0gIDxtYW5pYWdvb25AY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIHJldHVybiB2YWx1ZSBvZiBTaGFyZWRCdWZm ZXI6OmNyZWF0ZVdpdGhDb250ZW50c09mRmlsZSgpIG11c3QgaGF2ZSB2YWxpZCBtX3NpemUuCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MzMxMworCisg ICAgICAgICogcGxhdGZvcm0vcG9zaXgvU2hhcmVkQnVmZmVyUE9TSVguY3BwOgorICAgICAgICAo V2ViQ29yZTo6U2hhcmVkQnVmZmVyOjpjcmVhdGVXaXRoQ29udGVudHNPZkZpbGUpOgorCiAyMDEx LTA2LTIzICBaaGVueWFvIE1vICA8em1vQGdvb2dsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQg YnkgS2VubmV0aCBSdXNzZWxsLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v cG9zaXgvU2hhcmVkQnVmZmVyUE9TSVguY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcG9z aXgvU2hhcmVkQnVmZmVyUE9TSVguY3BwCmluZGV4IDZmNjZkNGQyODYzZmE3ODA1OTlhNjQ2Yjk1 OGJmYjg5MjhiYzVmYWIuLjE5MWNhNDZmYjY5Yzg1OWUyYTM2OTAwNGQwMDVhMzA3MGRiNmU4NWQg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3Bvc2l4L1NoYXJlZEJ1ZmZlclBP U0lYLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9wb3NpeC9TaGFyZWRCdWZmZXJQ T1NJWC5jcHAKQEAgLTY1LDYgKzY1LDggQEAgUGFzc1JlZlB0cjxTaGFyZWRCdWZmZXI+IFNoYXJl ZEJ1ZmZlcjo6Y3JlYXRlV2l0aENvbnRlbnRzT2ZGaWxlKGNvbnN0IFN0cmluZyYgZmkKICAgICB3 aGlsZSAoKGJ5dGVzUmVhZCA9IHJlYWQoZmQsIHJlc3VsdC0+bV9idWZmZXIuZGF0YSgpICsgdG90 YWxCeXRlc1JlYWQsIGJ5dGVzVG9SZWFkIC0gdG90YWxCeXRlc1JlYWQpKSA+IDApCiAgICAgICAg IHRvdGFsQnl0ZXNSZWFkICs9IGJ5dGVzUmVhZDsKIAorICAgIHJlc3VsdC0+bV9zaXplID0gdG90 YWxCeXRlc1JlYWQ7CisKICAgICBjbG9zZShmZCk7CiAKICAgICByZXR1cm4gdG90YWxCeXRlc1Jl YWQgPT0gYnl0ZXNUb1JlYWQgPyByZXN1bHQucmVsZWFzZSgpIDogMDsK 99961 2011-07-07 02:23:56 -0700 2011-07-07 10:29:32 -0700 Patch bug-63313-20110707182400.patch text/plain 1813 maniagoon U3VidmVyc2lvbiBSZXZpc2lvbjogOTA1NTAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA3NmM5YWY3Mzc4ZWQwZjZl NzljMmRmZDhiMmUzYWYwZWQyMjU1YjhlLi41Mjg3ZTI1NDhhYTZjMWE2MGRkODA5NTUxOTIwMjZk MjA0ZmU2MTc3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTEtMDctMDcgIEt5dXN1 biBLaW0gIDxtYW5pYWdvb25AY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgVGhlIHJldHVybiB2 YWx1ZSBvZiBTaGFyZWRCdWZmZXI6OmNyZWF0ZVdpdGhDb250ZW50c09mRmlsZSBtdXN0IGhhdmUg dmFsaWQgbV9zaXplLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NjMzMTMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIHBsYXRmb3JtL3Bvc2l4L1NoYXJlZEJ1ZmZlclBPU0lYLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OlNoYXJlZEJ1ZmZlcjo6Y3JlYXRlV2l0aENvbnRlbnRzT2ZGaWxlKToKKwogMjAxMS0w Ny0wNyAgT2xlZyBSb21hc2hpbiAgPHJvbWF4YUBnbWFpbC5jb20+CiAKICAgICAgICAgRml4IElu c3BlY3Rvckluc3RydW1lbnRhdGlvbiBjb21waWxhdGlvbi4KZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL3Bvc2l4L1NoYXJlZEJ1ZmZlclBPU0lYLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL3Bvc2l4L1NoYXJlZEJ1ZmZlclBPU0lYLmNwcAppbmRleCA2ZjY2ZDRkMjg2 M2ZhNzgwNTk5YTY0NmI5NThiZmI4OTI4YmM1ZmFiLi5lYmQ3MWJmZDZiNzRkYTMyOGZkZWIxZWEz ZDg3NTRlNzJiZGQxMDA4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9wb3Np eC9TaGFyZWRCdWZmZXJQT1NJWC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcG9z aXgvU2hhcmVkQnVmZmVyUE9TSVguY3BwCkBAIC01NywxNyArNTcsMTYgQEAgUGFzc1JlZlB0cjxT aGFyZWRCdWZmZXI+IFNoYXJlZEJ1ZmZlcjo6Y3JlYXRlV2l0aENvbnRlbnRzT2ZGaWxlKGNvbnN0 IFN0cmluZyYgZmkKICAgICAgICAgcmV0dXJuIDA7CiAgICAgfQogCi0gICAgUmVmUHRyPFNoYXJl ZEJ1ZmZlcj4gcmVzdWx0ID0gY3JlYXRlKCk7Ci0gICAgcmVzdWx0LT5tX2J1ZmZlci5ncm93KGJ5 dGVzVG9SZWFkKTsKKyAgICBWZWN0b3I8Y2hhcj4gYnVmZmVyKGJ5dGVzVG9SZWFkKTsKIAogICAg IHNpemVfdCB0b3RhbEJ5dGVzUmVhZCA9IDA7CiAgICAgc3NpemVfdCBieXRlc1JlYWQ7Ci0gICAg d2hpbGUgKChieXRlc1JlYWQgPSByZWFkKGZkLCByZXN1bHQtPm1fYnVmZmVyLmRhdGEoKSArIHRv dGFsQnl0ZXNSZWFkLCBieXRlc1RvUmVhZCAtIHRvdGFsQnl0ZXNSZWFkKSkgPiAwKQorICAgIHdo aWxlICgoYnl0ZXNSZWFkID0gcmVhZChmZCwgYnVmZmVyLmRhdGEoKSArIHRvdGFsQnl0ZXNSZWFk LCBieXRlc1RvUmVhZCAtIHRvdGFsQnl0ZXNSZWFkKSkgPiAwKQogICAgICAgICB0b3RhbEJ5dGVz UmVhZCArPSBieXRlc1JlYWQ7CiAKICAgICBjbG9zZShmZCk7CiAKLSAgICByZXR1cm4gdG90YWxC eXRlc1JlYWQgPT0gYnl0ZXNUb1JlYWQgPyByZXN1bHQucmVsZWFzZSgpIDogMDsKKyAgICByZXR1 cm4gdG90YWxCeXRlc1JlYWQgPT0gYnl0ZXNUb1JlYWQgPyBTaGFyZWRCdWZmZXI6OmFkb3B0VmVj dG9yKGJ1ZmZlcikgOiAwOwogfQogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCg==