167658 2017-01-31 12:18:29 -0800 Variable used after it's been WTFMove'd 2017-01-31 14:42:20 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 krollin krollin achristensen cdumez commit-queue dbates japhet oldest_to_newest 1271829 0 krollin 2017-01-31 12:18:29 -0800 SubresourceLoader::didReceiveDataOrBuffer WTFMoves "buffer": ResourceLoader::didReceiveDataOrBuffer(data, length, WTFMove(buffer), encodedDataLength, dataPayloadType); It later uses (the now empty) "buffer": m_resource->addData(buffer ? buffer->data() : data, buffer ? buffer->size() : length); With both "buffer" and "data" now being empty, m_resource->addData may not work as intended. m_resource is a CachedResource*, but I think I saw that it was pointing to a CachedRawResource in my debugging. This means that CachedRawResource::addData may be called with null values. This calls CachedRawResource::notifyClientsDataWasReceived, which will exit early when no data is passed in, skipping any actual notification. 1271830 1 cdumez 2017-01-31 12:25:44 -0800 Good find. 1271844 2 300249 krollin 2017-01-31 12:51:33 -0800 Created attachment 300249 Patch 1271847 3 300249 cdumez 2017-01-31 12:56:49 -0800 Comment on attachment 300249 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300249&action=review r=me > Source/WebCore/loader/SubresourceLoader.cpp:391 > + ResourceLoader::didReceiveDataOrBuffer(data, length, buffer.copyRef(), encodedDataLength, dataPayloadType); RefPtr has a copy constructor. I don't think it is actually needed here. 1271848 4 300249 cdumez 2017-01-31 12:57:10 -0800 Comment on attachment 300249 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300249&action=review >> Source/WebCore/loader/SubresourceLoader.cpp:391 >> + ResourceLoader::didReceiveDataOrBuffer(data, length, buffer.copyRef(), encodedDataLength, dataPayloadType); > > RefPtr has a copy constructor. I don't think it is actually needed here. I meant I don't think copyRef() is actually needed. 1271858 5 krollin 2017-01-31 13:18:47 -0800 It's needed because buffer is passed in as an rvalue reference and passed to the superclass as an rvalue reference. Without a WTFMove or a copyRef, we get: /Volumes/Data/dev/WebKit/branches/missing_logging/OpenSource/Source/WebCore/loader/SubresourceLoader.cpp:391:58: error: rvalue reference to type 'RefPtr<...>' cannot bind to lvalue of type 'RefPtr<...>' 1271906 6 300249 commit-queue 2017-01-31 14:42:14 -0800 Comment on attachment 300249 Patch Clearing flags on attachment: 300249 Committed r211449: <http://trac.webkit.org/changeset/211449> 1271907 7 commit-queue 2017-01-31 14:42:20 -0800 All reviewed patches have been landed. Closing bug. 300249 2017-01-31 12:51:33 -0800 2017-01-31 14:42:14 -0800 Patch bug-167658-20170131124922.patch text/plain 2813 krollin U3VidmVyc2lvbiBSZXZpc2lvbjogMjExMjkyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjlmMDViZDE4ZTFlY2Iw OTkwMmM3ODRjOGU0ZTEzODg5ZWRlYTM0My4uMjRmODRjZjI1MTQ4NTRlMTJjZjM1NTU2ODQxNTU2 NTQ4ZjRkMTI3NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE3LTAxLTMxICBLZWl0 aCBSb2xsaW4gIDxrcm9sbGluQGFwcGxlLmNvbT4KKworICAgICAgICBWYXJpYWJsZSB1c2VkIGFm dGVyIGl0J3MgYmVlbiBXVEZNb3ZlJ2QKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTE2NzY1OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFJhdGhlciB0aGFuIFdURk1vdmluZyB0aGUgdmFyaWFibGUgYXMgcGFy dCBvZiBwYXNzaW5nIGl0IHRvIGEgZnVuY3Rpb24KKyAgICAgICAgdGhhdCB0YWtlcyBhbiBSVmFs dWUtcmVmLCBjYWxsIGNvcHlSZWYgdG8gcHJvdmlkZSB0aGUgUlZhbHVlLXJlZi4gVGhpcworICAg ICAgICBwcmVzZXJ2ZXMgdGhlIG9yaWdpbmFsIHZhcmlhYmxlIGZvciBsYXRlciB1c2UgaW4gdGhl IGZ1bmN0aW9uLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyAtLSB0aGlzIGlzc3VlIHdhcyBmb3Vu ZCBieSBpbnNwZWN0aW9uLiBJdCdzIG5vdCBhcHBhcmVudAorICAgICAgICB3aGF0IHRoZSBzaWRl LWVmZmVjdCBvZiB0aGlzIGJ1ZyBpcyBvciBob3cgYSB0ZXN0IHdvdWxkIHZlcmlmeSBpdC4KKwor ICAgICAgICAqIGxvYWRlci9TdWJyZXNvdXJjZUxvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpTdWJyZXNvdXJjZUxvYWRlcjo6ZGlkUmVjZWl2ZURhdGFPckJ1ZmZlcik6CisKIDIwMTctMDEt MjcgIFphbGFuIEJ1anRhcyAgPHphbGFuQGFwcGxlLmNvbT4KIAogICAgICAgICBTaW1wbGUgbGlu ZSBsYXlvdXQ6IERvIG5vdCBiYWlsIG91dCBvbiAtd2Via2l0LWxpbmUtYm94LWNvbnRhaW46IGJs b2NrIGdseXBocyB1bmxlc3MgdGV4dCBvdmVyZmxvd3MgdmVydGljYWxseS4KZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJDb3JlL2xvYWRlci9TdWJyZXNvdXJjZUxvYWRlci5jcHAgYi9Tb3VyY2UvV2Vi Q29yZS9sb2FkZXIvU3VicmVzb3VyY2VMb2FkZXIuY3BwCmluZGV4IDhkNjViY2QwYmQ4ZDQwOGIx NzE0NzQxMTcxNTBhODNlM2QyMTNjYTUuLmIzNjNlMDUyZTkyYWU5OGVhNGY5ZDkzMjUxNDAwZThi ODI1ZGM3NmYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9TdWJyZXNvdXJjZUxv YWRlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL1N1YnJlc291cmNlTG9hZGVyLmNw cApAQCAtMzc3LDcgKzM3Nyw3IEBAIHZvaWQgU3VicmVzb3VyY2VMb2FkZXI6OmRpZFJlY2VpdmVC dWZmZXIoUmVmPFNoYXJlZEJ1ZmZlcj4mJiBidWZmZXIsIGxvbmcgbG9uZyBlCiAgICAgZGlkUmVj ZWl2ZURhdGFPckJ1ZmZlcihudWxscHRyLCAwLCBXVEZNb3ZlKGJ1ZmZlciksIGVuY29kZWREYXRh TGVuZ3RoLCBkYXRhUGF5bG9hZFR5cGUpOwogfQogCi12b2lkIFN1YnJlc291cmNlTG9hZGVyOjpk aWRSZWNlaXZlRGF0YU9yQnVmZmVyKGNvbnN0IGNoYXIqIGRhdGEsIGludCBsZW5ndGgsIFJlZlB0 cjxTaGFyZWRCdWZmZXI+JiYgcHJwQnVmZmVyLCBsb25nIGxvbmcgZW5jb2RlZERhdGFMZW5ndGgs IERhdGFQYXlsb2FkVHlwZSBkYXRhUGF5bG9hZFR5cGUpCit2b2lkIFN1YnJlc291cmNlTG9hZGVy OjpkaWRSZWNlaXZlRGF0YU9yQnVmZmVyKGNvbnN0IGNoYXIqIGRhdGEsIGludCBsZW5ndGgsIFJl ZlB0cjxTaGFyZWRCdWZmZXI+JiYgYnVmZmVyLCBsb25nIGxvbmcgZW5jb2RlZERhdGFMZW5ndGgs IERhdGFQYXlsb2FkVHlwZSBkYXRhUGF5bG9hZFR5cGUpCiB7CiAgICAgaWYgKG1fcmVzb3VyY2Ut PnJlc3BvbnNlKCkuaHR0cFN0YXR1c0NvZGUoKSA+PSA0MDAgJiYgIW1fcmVzb3VyY2UtPnNob3Vs ZElnbm9yZUhUVFBTdGF0dXNDb2RlRXJyb3JzKCkpCiAgICAgICAgIHJldHVybjsKQEAgLTM4Nyw5 ICszODcsOCBAQCB2b2lkIFN1YnJlc291cmNlTG9hZGVyOjpkaWRSZWNlaXZlRGF0YU9yQnVmZmVy KGNvbnN0IGNoYXIqIGRhdGEsIGludCBsZW5ndGgsIFJlZgogICAgIC8vIFJlZmVyZW5jZSB0aGUg b2JqZWN0IGluIHRoaXMgbWV0aG9kIHNpbmNlIHRoZSBhZGRpdGlvbmFsIHByb2Nlc3NpbmcgY2Fu IGRvCiAgICAgLy8gYW55dGhpbmcgaW5jbHVkaW5nIHJlbW92aW5nIHRoZSBsYXN0IHJlZmVyZW5j ZSB0byB0aGlzIG9iamVjdDsgb25lIGV4YW1wbGUgb2YgdGhpcyBpcyAzMjY2MjE2LgogICAgIFJl ZjxTdWJyZXNvdXJjZUxvYWRlcj4gcHJvdGVjdGVkVGhpcygqdGhpcyk7Ci0gICAgUmVmUHRyPFNo YXJlZEJ1ZmZlcj4gYnVmZmVyID0gcHJwQnVmZmVyOwotICAgIAotICAgIFJlc291cmNlTG9hZGVy OjpkaWRSZWNlaXZlRGF0YU9yQnVmZmVyKGRhdGEsIGxlbmd0aCwgV1RGTW92ZShidWZmZXIpLCBl bmNvZGVkRGF0YUxlbmd0aCwgZGF0YVBheWxvYWRUeXBlKTsKKworICAgIFJlc291cmNlTG9hZGVy OjpkaWRSZWNlaXZlRGF0YU9yQnVmZmVyKGRhdGEsIGxlbmd0aCwgYnVmZmVyLmNvcHlSZWYoKSwg ZW5jb2RlZERhdGFMZW5ndGgsIGRhdGFQYXlsb2FkVHlwZSk7CiAKICAgICBpZiAoIW1fbG9hZGlu Z011bHRpcGFydENvbnRlbnQpIHsKICAgICAgICAgaWYgKGF1dG8qIHJlc291cmNlRGF0YSA9IHRo aXMtPnJlc291cmNlRGF0YSgpKQo=