131203 2014-04-03 18:36:48 -0700 Crash in plugin process 2014-04-07 23:15:37 -0700 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 slewis webkit-unassigned andersca commit-queue ddkilzer slewis oldest_to_newest 997423 0 228573 slewis 2014-04-03 18:36:48 -0700 Created attachment 228573 patch <rdar://problem/16479432> 997425 1 commit-queue 2014-04-03 18:37:55 -0700 Attachment 228573 did not pass style-queue: ERROR: Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 997439 2 228573 ggaren 2014-04-03 19:04:43 -0700 Comment on attachment 228573 patch r=me You should explain in your ChangeLog that the old code crashed because it the C++ scope would delete its pluginController before the call to pluginDidBecomeHidden. Would it be better for this function to take a pluginInstanceID argument, instead of a raw (dangerous) PluginControllerProxy pointer? 998408 3 slewis 2014-04-07 20:51:44 -0700 committed http://trac.webkit.org/projects/webkit/changeset/166907 998449 4 228573 darin 2014-04-07 23:15:37 -0700 Comment on attachment 228573 patch View in context: https://bugs.webkit.org/attachment.cgi?id=228573&action=review > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:96 > + unsigned pluginInstanceID = pluginController->pluginInstanceID(); > { > - ASSERT(m_pluginControllers.contains(pluginController->pluginInstanceID())); > + ASSERT(m_pluginControllers.contains(pluginInstanceID)); > > - std::unique_ptr<PluginControllerProxy> pluginControllerUniquePtr = m_pluginControllers.take(pluginController->pluginInstanceID()); > + std::unique_ptr<PluginControllerProxy> pluginControllerUniquePtr = m_pluginControllers.take(pluginInstanceID); > ASSERT(pluginControllerUniquePtr.get() == pluginController); > } > > - pluginDidBecomeHidden(pluginController->pluginInstanceID()); > + pluginDidBecomeHidden(pluginInstanceID); Not really sure why the original code uses take. I would write this like this: unsigned instanceID = pluginController->pluginInstanceID(); ASSERT(m_pluginControllers.get(pluginInstanceID) == pluginController); m_pluginControllers.remove(instanceID); pluginDidBecomeHidden(pluginInstanceID); 228573 2014-04-03 18:36:48 -0700 2014-04-07 23:15:37 -0700 patch 0001-Crash-in-com.apple.WebKit.Plugin.Development-at-com..patch text/plain 2570 slewis RnJvbSBhMjFkZTE2MjMyNjlkMzg3NzQ5ODU5NjdiODBjNjhhNWVkNDllY2M1IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBTdGVwaGFuaWUgTGV3aXMgPHNsZXdpc0BhcHBsZS5jb20+CkRh dGU6IFRodSwgMyBBcHIgMjAxNCAxODozMjoyOSAtMDcwMApTdWJqZWN0OiBbUEFUQ0hdIENyYXNo IGluIGNvbS5hcHBsZS5XZWJLaXQuUGx1Z2luLkRldmVsb3BtZW50IGF0CiBjb20uYXBwbGUuV2Vi S2l0MjogV2ViS2l0OjpQbHVnaW5Db250cm9sbGVyUHJveHk6OnBsdWdpbkluc3RhbmNlSUQKIDxy ZGFyOi8vcHJvYmxlbS8xNjQ3OTQzMj4KClJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgoKKiBQ bHVnaW5Qcm9jZXNzL1dlYlByb2Nlc3NDb25uZWN0aW9uLmNwcDoKKFdlYktpdDo6V2ViUHJvY2Vz c0Nvbm5lY3Rpb246OnJlbW92ZVBsdWdpbkNvbnRyb2xsZXJQcm94eSk6Ci0tLQogU291cmNlL1dl YktpdDIvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfCAxMCArKysrKysr KysrCiBTb3VyY2UvV2ViS2l0Mi9QbHVnaW5Qcm9jZXNzL1dlYlByb2Nlc3NDb25uZWN0aW9uLmNw cCB8ICA3ICsrKystLS0KIDIgZmlsZXMgY2hhbmdlZCwgMTQgaW5zZXJ0aW9ucygrKSwgMyBkZWxl dGlvbnMoLSkKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cgYi9Tb3VyY2Uv V2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNTRkOWIwMi4uNzQ5Yjk5MSAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAt MSwzICsxLDEzIEBACisyMDE0LTA0LTAzICBTdGVwaGFuaWUgTGV3aXMgIDxzbGV3aXNAYXBwbGUu Y29tPgorCisgICAgICAgIENyYXNoIGluIGNvbS5hcHBsZS5XZWJLaXQuUGx1Z2luLkRldmVsb3Bt ZW50IGF0IGNvbS5hcHBsZS5XZWJLaXQyOiBXZWJLaXQ6OlBsdWdpbkNvbnRyb2xsZXJQcm94eTo6 cGx1Z2luSW5zdGFuY2VJRAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMTY0Nzk0MzI+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBQbHVnaW5Qcm9j ZXNzL1dlYlByb2Nlc3NDb25uZWN0aW9uLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViUHJvY2Vz c0Nvbm5lY3Rpb246OnJlbW92ZVBsdWdpbkNvbnRyb2xsZXJQcm94eSk6CisKIDIwMTQtMDQtMDIg IERlYW4gSmFja3NvbiAgPGRpbm9AYXBwbGUuY29tPgogCiAgICAgICAgIFtpT1NdIEV4cG9zZSBn ZW9sb2NhdGlvbiBwcm92aWRlciBvbiB0aGUgcHJvY2VzcyBwb29sCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViS2l0Mi9QbHVnaW5Qcm9jZXNzL1dlYlByb2Nlc3NDb25uZWN0aW9uLmNwcCBiL1NvdXJj ZS9XZWJLaXQyL1BsdWdpblByb2Nlc3MvV2ViUHJvY2Vzc0Nvbm5lY3Rpb24uY3BwCmluZGV4IGZl ODQxZTAuLmY5MmMxNTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1BsdWdpblByb2Nlc3Mv V2ViUHJvY2Vzc0Nvbm5lY3Rpb24uY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1BsdWdpblByb2Nl c3MvV2ViUHJvY2Vzc0Nvbm5lY3Rpb24uY3BwCkBAIC04NSwxNCArODUsMTUgQEAgdm9pZCBXZWJQ cm9jZXNzQ29ubmVjdGlvbjo6ZGVzdHJveVBsdWdpbkNvbnRyb2xsZXJQcm94eShQbHVnaW5Db250 cm9sbGVyUHJveHkqIHAKIAogdm9pZCBXZWJQcm9jZXNzQ29ubmVjdGlvbjo6cmVtb3ZlUGx1Z2lu Q29udHJvbGxlclByb3h5KFBsdWdpbkNvbnRyb2xsZXJQcm94eSogcGx1Z2luQ29udHJvbGxlciwg UGx1Z2luKiBwbHVnaW4pCiB7CisgICAgdW5zaWduZWQgcGx1Z2luSW5zdGFuY2VJRCA9IHBsdWdp bkNvbnRyb2xsZXItPnBsdWdpbkluc3RhbmNlSUQoKTsKICAgICB7Ci0gICAgICAgIEFTU0VSVCht X3BsdWdpbkNvbnRyb2xsZXJzLmNvbnRhaW5zKHBsdWdpbkNvbnRyb2xsZXItPnBsdWdpbkluc3Rh bmNlSUQoKSkpOworICAgICAgICBBU1NFUlQobV9wbHVnaW5Db250cm9sbGVycy5jb250YWlucyhw bHVnaW5JbnN0YW5jZUlEKSk7CiAKLSAgICAgICAgc3RkOjp1bmlxdWVfcHRyPFBsdWdpbkNvbnRy b2xsZXJQcm94eT4gcGx1Z2luQ29udHJvbGxlclVuaXF1ZVB0ciA9IG1fcGx1Z2luQ29udHJvbGxl cnMudGFrZShwbHVnaW5Db250cm9sbGVyLT5wbHVnaW5JbnN0YW5jZUlEKCkpOworICAgICAgICBz dGQ6OnVuaXF1ZV9wdHI8UGx1Z2luQ29udHJvbGxlclByb3h5PiBwbHVnaW5Db250cm9sbGVyVW5p cXVlUHRyID0gbV9wbHVnaW5Db250cm9sbGVycy50YWtlKHBsdWdpbkluc3RhbmNlSUQpOwogICAg ICAgICBBU1NFUlQocGx1Z2luQ29udHJvbGxlclVuaXF1ZVB0ci5nZXQoKSA9PSBwbHVnaW5Db250 cm9sbGVyKTsKICAgICB9CiAKLSAgICBwbHVnaW5EaWRCZWNvbWVIaWRkZW4ocGx1Z2luQ29udHJv bGxlci0+cGx1Z2luSW5zdGFuY2VJRCgpKTsKKyAgICBwbHVnaW5EaWRCZWNvbWVIaWRkZW4ocGx1 Z2luSW5zdGFuY2VJRCk7CiAKICAgICAvLyBJbnZhbGlkYXRlIGFsbCBvYmplY3RzIHJlbGF0ZWQg dG8gdGhpcyBwbHVnLWluLgogICAgIGlmIChwbHVnaW4pCi0tIAoxLjguNS4yIChBcHBsZSBHaXQt NDgpCgo=