135060 2014-07-18 12:15:13 -0700 Reduce the chances of a race condition when sharing SharedBuffer 2014-07-20 23:16:35 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 psolanki psolanki ap commit-queue darin kling koivisto psolanki simon.fraser thorton oldest_to_newest 1023309 0 psolanki 2014-07-18 12:15:13 -0700 We currently pass a SharedBuffer to ImageIO by creating a WebCoreSharedBufferData. This means ImageIO will access the SharedBuffer on a separate thread and this can cause data corruption bugs. While the actual fix is complex, we can at least reduce the time interval for this race condition to happen by setting the ShardBuffer size after the data is copied in SharedBuffer::append(). <rdar://problem/17729444> 1023313 1 235136 psolanki 2014-07-18 12:25:32 -0700 Created attachment 235136 Patch 1023331 2 235136 ggaren 2014-07-18 13:02:38 -0700 Comment on attachment 235136 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235136&action=review > Source/WebCore/ChangeLog:17 > + cector has finished appending. cector => vector > Source/WebCore/platform/SharedBuffer.cpp:362 > - m_size += length; > if (m_buffer.isEmpty()) > m_buffer.reserveInitialCapacity(length); > m_buffer.append(data, length); > + m_size += length; What is the plan for fixing the race condition? It's hard to evaluate whether this change is right. If you wanted to make this data thread-consistent, you would probably do something like a store barrier before setting m_size. It's not really coherent to move the setting of m_size because, in an SMP environment, all code runs out of order anyway. 1023358 3 psolanki 2014-07-18 13:55:38 -0700 (In reply to comment #2) > (From update of attachment 235136 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235136&action=review > > > Source/WebCore/ChangeLog:17 > > + cector has finished appending. > > cector => vector > > > Source/WebCore/platform/SharedBuffer.cpp:362 > > - m_size += length; > > if (m_buffer.isEmpty()) > > m_buffer.reserveInitialCapacity(length); > > m_buffer.append(data, length); > > + m_size += length; > > What is the plan for fixing the race condition? <rdar://17470655> tracks this. I also filed <https://bugs.webkit.org/show_bug.cgi?id=135069>. > It's hard to evaluate whether this change is right. If you wanted to make this data thread-consistent, you would probably do something like a store barrier before setting m_size. It's not really coherent to move the setting of m_size because, in an SMP environment, all code runs out of order anyway. I am not trying to fix the actual race condition - merely trying to reduce the time where it can happen. Testing indicates that the fix helps. It is possible that the compiler could reorder the instructions but my testing shows improvements from previous behavior. At the very least, this change should not make anything worse than before. 1023416 4 psolanki 2014-07-18 18:23:30 -0700 So, would it be okay to make this change? Or should the patch be r-? 1023499 5 darin 2014-07-19 22:49:45 -0700 What’s the timeline on the real fix? I’m not sure making this tweak to reduce the frequency of the bug’s symptom is a good thing to do, but it’s hard to see how it would do harm. 1023609 6 235136 darin 2014-07-20 20:58:50 -0700 Comment on attachment 235136 Patch Lets get this change in even though it’s not a real fix. I’m still not sure about the strategy; it seems strange to do this instead of fixing the bug. 1023610 7 235136 commit-queue 2014-07-20 21:30:38 -0700 Comment on attachment 235136 Patch Clearing flags on attachment: 235136 Committed r171289: <http://trac.webkit.org/changeset/171289> 1023611 8 commit-queue 2014-07-20 21:30:42 -0700 All reviewed patches have been landed. Closing bug. 1023620 9 psolanki 2014-07-20 23:16:35 -0700 (In reply to comment #5) > What’s the timeline on the real fix? I’m not sure making this tweak to reduce the frequency of the bug’s symptom is a good thing to do, but it’s hard to see how it would do harm. I have the fix almost working - I am running into issues with custom fonts that I am trying to figure out. The bug I see is identical to what was seen in bug 115131. CGFont/CGFontDataProvider is not happy with my code changes in WebCoreSharedBufferData. (In reply to comment #6) > (From update of attachment 235136 [details]) > Lets get this change in even though it’s not a real fix. I’m still not sure about the strategy; it seems strange to do this instead of fixing the bug. This was a quick change that at least makes images load. We should have a clean way of sharing data with ImageIO but my changes started looking risky - not to mention the perf impact it might have due to more copying. I'll upload my wip patches to bug 135069 while I continue testing. Thanks for the review! 235136 2014-07-18 12:25:32 -0700 2014-07-20 21:30:38 -0700 Patch bug-135060.patch text/plain 2097 psolanki ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3NTIyY2VlLi44NTMzODM1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjYg QEAKKzIwMTQtMDctMTggIFByYXRpayBTb2xhbmtpICA8cHNvbGFua2lAYXBwbGUuY29tPgorCisg ICAgICAgIFJlZHVjZSB0aGUgY2hhbmNlcyBvZiBhIHJhY2UgY29uZGl0aW9uIHdoZW4gc2hhcmlu ZyBTaGFyZWRCdWZmZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTEzNTA2MAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMTc3Mjk0NDQ+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2UgY3VycmVudGx5IHBh c3MgYSBTaGFyZWRCdWZmZXIgd3JhcHBlZCBpbiBXZWJDb3JlU2hhcmVkQnVmZmVyRGF0YSB0byBJ bWFnZUlPIGZvciBpbWFnZQorICAgICAgICBkZWNvZGluZy4gVGhpcyBpcyBub3QgdGhyZWFkIHNh ZmUgc2luY2UgSW1hZ2VJTyB3aWxsIGFjY2VzcyB0aGlzIGJ1ZmZlciBvbiBhIHNlcGFyYXRlCisg ICAgICAgIHRocmVhZC4gV2UgYWNjZXNzIFNoYXJlZEJ1ZmZlcjo6YnVmZmVyKCkgb24gdGhlIG90 aGVyIHRocmVhZCB3aGljaCByZXNpemVzIHRoZSBWZWN0b3IKKyAgICAgICAgbV9idWZmZXIgaWYg bV9zaXplIGlzIGdyZWF0ZXIgdGhhbiB0aGUgdmVjdG9yIHNpemUuIFNpbmNlIHRoZSBjb2RlIGlu IFNoYXJlZEJ1ZmZlcjo6YXBwZW5kKCkKKyAgICAgICAgc2V0cyBtX3NpemUgYmVmb3JlIGFwcGVu ZGluZyB0aGUgZGF0YSB0byB0aGUgYnVmZmVyLCBtX3NpemUgaXMgb3V0IG9mIHN5bmMgd2l0aCB0 aGUgbV9idWZmZXIKKyAgICAgICAgc2l6ZSBmb3IgdGhlIGVudGlyZSBkdXJhdGlvbiBvZiB0aGUg VmVjdG9yIGFwcGVuZCB3aGljaCBjb3VsZCBiZSBkb2luZyBhIGxvdCBvZiBjb3B5aW5nIGlmCisg ICAgICAgIHRoZSByZXNvdXJjZSBpcyBsYXJnZS4gV2hpbGUgdGhpcyBjaGFuZ2UgZG9lcyBub3Qg Zml4IHRoZSByYWNlIGNvbmRpdGlvbiwgd2UgY2FuIGF0IGxlYXN0CisgICAgICAgIHJlZHVjZSB0 aGUgY2hhbmNlcyBvZiBTaGFyZWRCdWZmZXI6OmJ1ZmZlcigpIGNhbGxpbmcgcmVzaXplKCkgYnkg c2V0dGluZyBtX3NpemUgYWZ0ZXIgdGhlCisgICAgICAgIGNlY3RvciBoYXMgZmluaXNoZWQgYXBw ZW5kaW5nLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyBiZWNhdXNlIG5vIGZ1bmN0aW9uYWwgY2hh bmdlcy4KKworICAgICAgICAqIHBsYXRmb3JtL1NoYXJlZEJ1ZmZlci5jcHA6CisgICAgICAgIChX ZWJDb3JlOjpTaGFyZWRCdWZmZXI6OmFwcGVuZCk6CisKIDIwMTQtMDctMTcgIFZpbmVldCBDaGF1 ZGhhcnkgIDxjb2RlLnZpbmVldEBnbWFpbC5jb20+CiAKICAgICAgICAgW0dPYmplY3RdIFN0cmlj dFR5cGVDaGVja2luZyBleHRlbmRlZCBhdHRyaWJ1dGUgZmFpbHMgZm9yIG1ldGhvZHMgd2l0aCBz ZXF1ZW5jZTxUPi4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1NoYXJlZEJ1 ZmZlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9TaGFyZWRCdWZmZXIuY3BwCmluZGV4 IDlkZTQ4MjEuLjY5MmJjNTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1No YXJlZEJ1ZmZlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2hhcmVkQnVmZmVy LmNwcApAQCAtMzU2LDEwICszNTYsMTAgQEAgdm9pZCBTaGFyZWRCdWZmZXI6OmFwcGVuZChjb25z dCBjaGFyKiBkYXRhLCB1bnNpZ25lZCBsZW5ndGgpCiAgICAgICAgIGJ5dGVzVG9Db3B5ID0gc3Rk OjptaW4obGVuZ3RoLCBzZWdtZW50U2l6ZSk7CiAgICAgfQogI2Vsc2UKLSAgICBtX3NpemUgKz0g bGVuZ3RoOwogICAgIGlmIChtX2J1ZmZlci5pc0VtcHR5KCkpCiAgICAgICAgIG1fYnVmZmVyLnJl c2VydmVJbml0aWFsQ2FwYWNpdHkobGVuZ3RoKTsKICAgICBtX2J1ZmZlci5hcHBlbmQoZGF0YSwg bGVuZ3RoKTsKKyAgICBtX3NpemUgKz0gbGVuZ3RoOwogI2VuZGlmCiB9CiAK