113430 2013-03-27 11:43:09 -0700 [wk2] REGRESSION (r125500): WebFrameLoaderClient's active m_pluginView can be zeroed by PluginDocuments leaving the page cache 2013-05-03 04:46:55 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton thorton ap beidson eric sam tsepez oldest_to_newest 864740 0 thorton 2013-03-27 11:43:09 -0700 http://trac.webkit.org/changeset/125500 made a change that purported to be Chromium only (it even had a [chromium] tag!) but actually changed the behavior for WebKit2 as well: It now zeroes the FrameLoaderClient's redirecting-PluginView in PluginDocument::detach(), which can happen *way later* in the case that the PluginDocument went into the page cache, clearing a legitimate-and-different PluginView from (Web)FrameLoaderClient. This change also made sure not to change behavior for other ports, ignoring the new redirectDataToPlugin(0) case, but missed WebKit2 (possibly intentionally because m_pluginView is a RefPtr for WK2 and not for most other non-Chromium ports). We already clear the m_pluginView RefPtr when the load finishes or fails, so I think we can safely early-return in the redirectDataToPlugin(0) case just like all the other ports, returning to our behavior before r125500. <rdar://problem/12729564> 864744 1 195369 thorton 2013-03-27 11:47:53 -0700 Created attachment 195369 patch 865679 2 195369 beidson 2013-03-28 12:02:19 -0700 Comment on attachment 195369 patch Sloppy patch originally :( Fix seems reasonable. 865753 3 thorton 2013-03-28 13:22:54 -0700 http://trac.webkit.org/changeset/147152 865775 4 195369 darin 2013-03-28 14:02:24 -0700 Comment on attachment 195369 patch View in context: https://bugs.webkit.org/attachment.cgi?id=195369&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1350 > - m_pluginView = static_cast<PluginView*>(pluginWidget); > + if (m_pluginView) > + m_pluginView = static_cast<PluginView*>(pluginWidget); I think you mean: if (pluginWidget) not if (m_pluginView) 865776 5 thorton 2013-03-28 14:04:06 -0700 (In reply to comment #4) > (From update of attachment 195369 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195369&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1350 > > - m_pluginView = static_cast<PluginView*>(pluginWidget); > > + if (m_pluginView) > > + m_pluginView = static_cast<PluginView*>(pluginWidget); > > I think you mean: > > if (pluginWidget) > > not > > if (m_pluginView) Woah, you're right. How I manage to screw up one line patches I'll never know. 865777 6 beidson 2013-03-28 14:08:44 -0700 And me too :( 865779 7 thorton 2013-03-28 14:12:30 -0700 Reopening for correct fix. 865781 8 195638 thorton 2013-03-28 14:13:24 -0700 Created attachment 195638 patch 865839 9 thorton 2013-03-28 15:06:27 -0700 (In reply to comment #8) > Created an attachment (id=195638) [details] > patch http://trac.webkit.org/changeset/147168 195369 2013-03-27 11:47:53 -0700 2013-03-28 14:13:24 -0700 patch loading.diff text/plain 2235 thorton ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA5ODI5MDc3Li42YzNmZDk4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMg QEAKKzIwMTMtMDMtMjcgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisK KyAgICAgICAgW3drMl0gUkVHUkVTU0lPTiAocjEyNTUwMCk6IFdlYkZyYW1lTG9hZGVyQ2xpZW50 J3MgYWN0aXZlIG1fcGx1Z2luVmlldyBjYW4gYmUgemVyb2VkIGJ5IFBsdWdpbkRvY3VtZW50cyBs ZWF2aW5nIHRoZSBwYWdlIGNhY2hlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xMTM0MzAKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzEyNzI5NTY0Pgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIElnbm9yZSBy ZWRpcmVjdERhdGFUb1BsdWdpbigwKSwgd2hpY2ggaXMgb25seSBjYWxsZWQgZnJvbSBQbHVnaW5E b2N1bWVudDo6ZGV0YWNoKCksCisgICAgICAgIGFuZCBvbmx5IGFmdGVyIHIxMjU1MDAuIE90aGVy d2lzZSwgUGx1Z2luRG9jdW1lbnRzIHRoYXQgYXJlIHB1dCBpbnRvIHRoZSBwYWdlIGNhY2hlCisg ICAgICAgIGJ1dCBsZWF2ZSBsYXRlciBjYW4gY2xlYXIgYSB0b3RhbGx5IGRpZmZlcmVudCBhbmQg bGVnaXRpbWF0ZSBQbHVnaW5WaWV3IGZyb20gdGhlCisgICAgICAgIFdlYkZyYW1lTG9hZGVyQ2xp ZW50LCBjYXVzaW5nIGRhdGEgdG8gYmUgbG9zdCBhbmQgUGx1Z2luRG9jdW1lbnQgbG9hZHMgdG8g c3RhbGwuCisKKyAgICAgICAgVGhpcyByZXN0b3JlcyBvdXIgYmVoYXZpb3IgdG8gdGhlIHByZS1y MTI1NTAwIHN0YXRlLCB3aGljaCBzaG91bGQgYmUgZmluZSwgYXMgd2UKKyAgICAgICAgY2xlYXIg V2ViRnJhbWVMb2FkZXJDbGllbnQncyBtX3BsdWdpblZpZXcgd2hlbiB0aGUgbG9hZCBmYWlscyBv ciBmaW5pc2hlcy4gVGhpcworICAgICAgICBtYXRjaGVzIHRoZSBjaGFuZ2VzIG1hZGUgaW4gcjEy NTUwMCBmb3IgZXZlcnkgcGxhdGZvcm0gb3RoZXIgdGhhbiBDaHJvbWl1bSBhbmQgV2ViS2l0Mi4K KworICAgICAgICAqIFdlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvV2ViRnJhbWVMb2FkZXJDbGll bnQuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJGcmFtZUxvYWRlckNsaWVudDo6cmVkaXJlY3RE YXRhVG9QbHVnaW4pOgorCiAyMDEzLTAzLTI1ICBNYXhpbSBFcm1pbG92ICA8bWF4aW0uZXJtaWxv dkBjYW5vbmljYWwuY29tPgogCiAgICAgICAgIFtRdF0gUW1sIFdlYktpdCBuZWVkcyBwcmVmZXJl bmNlcyB0byBjaGFuZ2Ugc2VjdXJpdHkga25vYnMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQy L1dlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvV2ViRnJhbWVMb2FkZXJDbGllbnQuY3BwIGIvU291 cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJDb3JlU3VwcG9ydC9XZWJGcmFtZUxvYWRlckNsaWVu dC5jcHAKaW5kZXggMTk3NzM0NC4uZjgzMzMyMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIv V2ViUHJvY2Vzcy9XZWJDb3JlU3VwcG9ydC9XZWJGcmFtZUxvYWRlckNsaWVudC5jcHAKKysrIGIv U291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJDb3JlU3VwcG9ydC9XZWJGcmFtZUxvYWRlckNs aWVudC5jcHAKQEAgLTEzNDYsNyArMTM0Niw4IEBAIHZvaWQgV2ViRnJhbWVMb2FkZXJDbGllbnQ6 OnJlY3JlYXRlUGx1Z2luKFdpZGdldCogd2lkZ2V0KQogCiB2b2lkIFdlYkZyYW1lTG9hZGVyQ2xp ZW50OjpyZWRpcmVjdERhdGFUb1BsdWdpbihXaWRnZXQqIHBsdWdpbldpZGdldCkKIHsKLSAgICBt X3BsdWdpblZpZXcgPSBzdGF0aWNfY2FzdDxQbHVnaW5WaWV3Kj4ocGx1Z2luV2lkZ2V0KTsKKyAg ICBpZiAobV9wbHVnaW5WaWV3KQorICAgICAgICBtX3BsdWdpblZpZXcgPSBzdGF0aWNfY2FzdDxQ bHVnaW5WaWV3Kj4ocGx1Z2luV2lkZ2V0KTsKIH0KIAogUGFzc1JlZlB0cjxXaWRnZXQ+IFdlYkZy YW1lTG9hZGVyQ2xpZW50OjpjcmVhdGVKYXZhQXBwbGV0V2lkZ2V0KGNvbnN0IEludFNpemUmIHBs dWdpblNpemUsIEhUTUxBcHBsZXRFbGVtZW50KiBhcHBsZXRFbGVtZW50LCBjb25zdCBLVVJMJiwg Y29uc3QgVmVjdG9yPFN0cmluZz4mIHBhcmFtTmFtZXMsIGNvbnN0IFZlY3RvcjxTdHJpbmc+JiBw YXJhbVZhbHVlcykK 195638 2013-03-28 14:13:24 -0700 2013-05-03 04:46:55 -0700 patch fix.diff text/plain 1612 thorton SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0NzE1NikKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtNCw2ICs0LDIxIEBACiAgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTM0MzAKICAgICAgICAgPHJkYXI6Ly9wcm9i bGVtLzEyNzI5NTY0PgogCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIGh0dHA6Ly90cmFjLndlYmtpdC5vcmcvY2hhbmdlc2V0LzE0NzE1MiB3YXMgd3Jvbmcs IGFuZCBzaG91bGQgaGF2ZSBiZWVuCisgICAgICAgIChhcyB0aGUgQ2hhbmdlTG9nIHN0YXRlZCkg dGVzdGluZyB0aGUgYXJndW1lbnQgdG8gcmVkaXJlY3REYXRhVG9QbHVnaW4sCisgICAgICAgIG5v dCB0aGUgdmFsdWUgb2YgbV9wbHVnaW5WaWV3LgorCisgICAgICAgICogV2ViUHJvY2Vzcy9XZWJD b3JlU3VwcG9ydC9XZWJGcmFtZUxvYWRlckNsaWVudC5jcHA6CisgICAgICAgIChXZWJLaXQ6Oldl YkZyYW1lTG9hZGVyQ2xpZW50OjpyZWRpcmVjdERhdGFUb1BsdWdpbik6CisKKzIwMTMtMDMtMjgg IFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisKKyAgICAgICAgW3drMl0g UkVHUkVTU0lPTiAocjEyNTUwMCk6IFdlYkZyYW1lTG9hZGVyQ2xpZW50J3MgYWN0aXZlIG1fcGx1 Z2luVmlldyBjYW4gYmUgemVyb2VkIGJ5IFBsdWdpbkRvY3VtZW50cyBsZWF2aW5nIHRoZSBwYWdl IGNhY2hlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MTM0MzAKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzEyNzI5NTY0PgorCiAgICAgICAgIFJldmll d2VkIGJ5IEJyYWR5IEVpZHNvbi4KIAogICAgICAgICBJZ25vcmUgcmVkaXJlY3REYXRhVG9QbHVn aW4oMCksIHdoaWNoIGlzIG9ubHkgY2FsbGVkIGZyb20gUGx1Z2luRG9jdW1lbnQ6OmRldGFjaCgp LApJbmRleDogU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJDb3JlU3VwcG9ydC9XZWJGcmFt ZUxvYWRlckNsaWVudC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvV2ViUHJvY2Vz cy9XZWJDb3JlU3VwcG9ydC9XZWJGcmFtZUxvYWRlckNsaWVudC5jcHAJKHJldmlzaW9uIDE0NzE1 NikKKysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvV2ViRnJhbWVM b2FkZXJDbGllbnQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMzQ2LDcgKzEzNDYsNyBAQAogCiB2 b2lkIFdlYkZyYW1lTG9hZGVyQ2xpZW50OjpyZWRpcmVjdERhdGFUb1BsdWdpbihXaWRnZXQqIHBs dWdpbldpZGdldCkKIHsKLSAgICBpZiAobV9wbHVnaW5WaWV3KQorICAgIGlmIChwbHVnaW5XaWRn ZXQpCiAgICAgICAgIG1fcGx1Z2luVmlldyA9IHN0YXRpY19jYXN0PFBsdWdpblZpZXcqPihwbHVn aW5XaWRnZXQpOwogfQogCg==