114702 2013-04-16 14:57:43 -0700 PlugIn Snapshotting: Crashes refreshing non-main-frame PDFPlugins 2013-04-16 15:57:16 -0700 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton thorton commit-queue dino esprehn+autocc oldest_to_newest 876883 0 thorton 2013-04-16 14:57:43 -0700 If a plugin shouldAlwaysAutoStart() and is also detected as primary, setIsPrimarySnapshottedPlugIn can switch displayState() to Restarting and reattach, but then when the plugin is created, we're pushed forward to Playing, and things go very very poorly from there on out (we keep two plugin instances around, etc.) This is reproducible with PDFPlugin, clicking Chase.com account statements. <rdar://problem/13542020> 876891 1 198439 thorton 2013-04-16 15:09:48 -0700 Created attachment 198439 patch 876894 2 198439 dino 2013-04-16 15:15:47 -0700 Comment on attachment 198439 patch View in context: https://bugs.webkit.org/attachment.cgi?id=198439&action=review > Source/WebCore/html/HTMLPlugInImageElement.cpp:86 > + , m_plugInWasCreated(false) Could we use the value of the SnapshotDecision enum to avoid this? > Source/WebCore/html/HTMLPlugInImageElement.cpp:519 > + if (m_plugInWasCreated) { > + LOG(Plugins, "%p Plug-in was detected as the primary element in the page. Restart.", this); > + restartSnapshottedPlugIn(); > + restartSimilarPlugIns(); > + } else { > + LOG(Plugins, "%p Plug-in was detected as the primary element in the page, but is not yet created. Will restart later.", this); > + m_deferredPromotionToPrimaryPlugIn = true; > + } And we could set snapshotDecision to NeverSnapshot here. 876913 3 thorton 2013-04-16 15:39:20 -0700 (In reply to comment #2) > (From update of attachment 198439 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198439&action=review > > > Source/WebCore/html/HTMLPlugInImageElement.cpp:86 > > + , m_plugInWasCreated(false) > > Could we use the value of the SnapshotDecision enum to avoid this? Nope, because we need to know if we are strictly between willCreatePlugIn and didCreatePlugIn, which isn't something we can tell from SnapshotDecision. > > Source/WebCore/html/HTMLPlugInImageElement.cpp:519 > > + if (m_plugInWasCreated) { > > + LOG(Plugins, "%p Plug-in was detected as the primary element in the page. Restart.", this); > > + restartSnapshottedPlugIn(); > > + restartSimilarPlugIns(); > > + } else { > > + LOG(Plugins, "%p Plug-in was detected as the primary element in the page, but is not yet created. Will restart later.", this); > > + m_deferredPromotionToPrimaryPlugIn = true; > > + } > > And we could set snapshotDecision to NeverSnapshot here. Nope, because nothing ever consults snapshotDecision again after willCreatePlugin. And even if we added a check for snapshotDecision in didCreatePlugin, we wouldn't know to restart *similar* plugins as well (like we do for primary plugins). 876931 4 198439 dino 2013-04-16 15:52:45 -0700 Comment on attachment 198439 patch View in context: https://bugs.webkit.org/attachment.cgi?id=198439&action=review >>> Source/WebCore/html/HTMLPlugInImageElement.cpp:86 >>> + , m_plugInWasCreated(false) >> >> Could we use the value of the SnapshotDecision enum to avoid this? > > Nope, because we need to know if we are strictly between willCreatePlugIn and didCreatePlugIn, which isn't something we can tell from SnapshotDecision. Bummer. >>> Source/WebCore/html/HTMLPlugInImageElement.cpp:519 >>> + } >> >> And we could set snapshotDecision to NeverSnapshot here. > > Nope, because nothing ever consults snapshotDecision again after willCreatePlugin. And even if we added a check for snapshotDecision in didCreatePlugin, we wouldn't know to restart *similar* plugins as well (like we do for primary plugins). Double bummer. 876934 5 thorton 2013-04-16 15:57:16 -0700 Bummer x2, indeed :( https://trac.webkit.org/changeset/148560 Thanks, Dean! 198439 2013-04-16 15:09:48 -0700 2013-04-16 15:52:45 -0700 patch pdf.diff text/plain 5858 thorton ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA1MTFhZjk0Li4zZTM3Y2NkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMzQg QEAKKzIwMTMtMDQtMTYgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisK KyAgICAgICAgUGx1Z0luIFNuYXBzaG90dGluZzogQ3Jhc2hlcyByZWZyZXNoaW5nIG5vbi1tYWlu LWZyYW1lIFBERlBsdWdpbnMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTExNDcwMgorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMTM1NDIwMjA+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSWY6CisKKyAgICAg ICAgICAgIGEpIGEgcGx1Z2luIGZhaWxzIGFsbCB0aGUgdGVzdHMgaW4gd2lsbENyZWF0ZVBsdWdJ biwgc28gaXQgaXMgV2FpdGluZ0ZvclNuYXBzaG90IHRoZXJlCisgICAgICAgICAgICBiKSBwcmlt YXJ5IHBsdWdpbiBkZXRlY3Rpb24gYXR0ZW1wdHMgdG8gcmVzdGFydCBhIHBsdWdpbiBiZXR3ZWVu IHRoZQorICAgICAgICAgICAgICAgdGltZSB0aGF0IHdpbGxDcmVhdGVQbHVnSW4gYW5kIGRpZENy ZWF0ZVBsdWdJbiBmaXJlCisgICAgICAgICAgICBjKSB3aGVuIGRpZENyZWF0ZVBsdWdJbiBmaXJl cywgc2hvdWxkQWx3YXlzQXV0b1N0YXJ0KCkgcmV0dXJucyB0cnVlLAorICAgICAgICAgICAgICAg YmVjYXVzZSB0aGUgcGx1Zy1pbiBpcyB3aGl0ZWxpc3RlZCwKKworICAgICAgICB3ZSBlbmQgdXAg cmVhdHRhY2goKWluZyBhbmQgZ29pbmcgdG8gUmVzdGFydGVkIHN0YXRlLCBhbmQgdGhlbiBnb2lu ZyBzdHJhaWdodAorICAgICAgICB0byBQbGF5aW5nIHN0YXRlIGluIGRpZENyZWF0ZVBsdWdJbi4K KworICAgICAgICBJbnN0ZWFkLCBwcmltYXJ5IHBsdWdpbiBwcm9tb3Rpb24gc2hvdWxkIGJlIGRl ZmVycmVkIHVudGlsIGFmdGVyIHRoZSBwbHVnaW4gaXMgY3JlYXRlZCwKKyAgICAgICAgc28gdGhh dCB3ZSBjYW4gdGFrZSB0aGF0IG9uZSBsYXN0IGJpdCBvZiBpbmZvcm1hdGlvbiAoc2hvdWxkQWx3 YXlzQXV0b1N0YXJ0KSBpbnRvIGFjY291bnQKKyAgICAgICAgYmVmb3JlIHJlc3RhcnRpbmcgYW5k IHJlYXR0YWNoaW5nIHRoZSBwbHVnLWluLgorCisgICAgICAgICogaHRtbC9IVE1MUGx1Z0luSW1h Z2VFbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxQbHVnSW5JbWFnZUVsZW1lbnQ6 OkhUTUxQbHVnSW5JbWFnZUVsZW1lbnQpOgorICAgICAgICAoV2ViQ29yZTo6SFRNTFBsdWdJbklt YWdlRWxlbWVudDo6c2V0SXNQcmltYXJ5U25hcHNob3R0ZWRQbHVnSW4pOgorICAgICAgICAoV2Vi Q29yZTo6SFRNTFBsdWdJbkltYWdlRWxlbWVudDo6cmVzdGFydFNuYXBzaG90dGVkUGx1Z0luKToK KyAgICAgICAgKFdlYkNvcmU6OkhUTUxQbHVnSW5JbWFnZUVsZW1lbnQ6OnN1YmZyYW1lTG9hZGVy V2lsbENyZWF0ZVBsdWdJbik6CisgICAgICAgIChXZWJDb3JlOjpIVE1MUGx1Z0luSW1hZ2VFbGVt ZW50OjpzdWJmcmFtZUxvYWRlckRpZENyZWF0ZVBsdWdJbik6CisgICAgICAgICogaHRtbC9IVE1M UGx1Z0luSW1hZ2VFbGVtZW50Lmg6CisKIDIwMTMtMDQtMTYgIEFudGhvbnkgU2NpYW4gIDxhc2Np YW5AYmxhY2tiZXJyeS5jb20+CiAKICAgICAgICAgW0JsYWNrQmVycnldIExheWVyVGV4dHVyZTog Y2hlY2sgaWYgdGhlIGdyYXBoaWNzIGNvbnRleHQgaXMgTlVMTApkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvaHRtbC9IVE1MUGx1Z0luSW1hZ2VFbGVtZW50LmNwcCBiL1NvdXJjZS9XZWJDb3Jl L2h0bWwvSFRNTFBsdWdJbkltYWdlRWxlbWVudC5jcHAKaW5kZXggNzUzMDAxMi4uNTQxNWQ4MSAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MUGx1Z0luSW1hZ2VFbGVtZW50LmNw cAorKysgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxQbHVnSW5JbWFnZUVsZW1lbnQuY3BwCkBA IC04Myw2ICs4Myw4IEBAIEhUTUxQbHVnSW5JbWFnZUVsZW1lbnQ6OkhUTUxQbHVnSW5JbWFnZUVs ZW1lbnQoY29uc3QgUXVhbGlmaWVkTmFtZSYgdGFnTmFtZSwgRG9jCiAgICAgLCBtX2NyZWF0ZWRE dXJpbmdVc2VyR2VzdHVyZShTY3JpcHRDb250cm9sbGVyOjpwcm9jZXNzaW5nVXNlckdlc3R1cmUo KSkKICAgICAsIG1faXNSZXN0YXJ0ZWRQbHVnaW4oZmFsc2UpCiAgICAgLCBtX25lZWRzQ2hlY2tG b3JTaXplQ2hhbmdlKGZhbHNlKQorICAgICwgbV9wbHVnSW5XYXNDcmVhdGVkKGZhbHNlKQorICAg ICwgbV9kZWZlcnJlZFByb21vdGlvblRvUHJpbWFyeVBsdWdJbihmYWxzZSkKICAgICAsIG1fc25h cHNob3REZWNpc2lvbihTbmFwc2hvdE5vdFlldERlY2lkZWQpCiB7CiAgICAgc2V0SGFzQ3VzdG9t U3R5bGVDYWxsYmFja3MoKTsKQEAgLTUwNywxNyArNTA5LDIzIEBAIHZvaWQgSFRNTFBsdWdJbklt YWdlRWxlbWVudDo6c2V0SXNQcmltYXJ5U25hcHNob3R0ZWRQbHVnSW4oYm9vbCBpc1ByaW1hcnlT bmFwc2hvCiAgICAgICAgIHJldHVybjsKIAogICAgIGlmIChpc1ByaW1hcnlTbmFwc2hvdHRlZFBs dWdJbikgewotICAgICAgICBMT0coUGx1Z2lucywgIiVwIFBsdWctaW4gd2FzIGRldGVjdGVkIGFz IHRoZSBwcmltYXJ5IGVsZW1lbnQgaW4gdGhlIHBhZ2UuIFJlc3RhcnQuIiwgdGhpcyk7Ci0gICAg ICAgIHJlc3RhcnRTbmFwc2hvdHRlZFBsdWdJbigpOwotICAgICAgICByZXN0YXJ0U2ltaWxhclBs dWdJbnMoKTsKKyAgICAgICAgaWYgKG1fcGx1Z0luV2FzQ3JlYXRlZCkgeworICAgICAgICAgICAg TE9HKFBsdWdpbnMsICIlcCBQbHVnLWluIHdhcyBkZXRlY3RlZCBhcyB0aGUgcHJpbWFyeSBlbGVt ZW50IGluIHRoZSBwYWdlLiBSZXN0YXJ0LiIsIHRoaXMpOworICAgICAgICAgICAgcmVzdGFydFNu YXBzaG90dGVkUGx1Z0luKCk7CisgICAgICAgICAgICByZXN0YXJ0U2ltaWxhclBsdWdJbnMoKTsK KyAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgICAgIExPRyhQbHVnaW5zLCAiJXAgUGx1Zy1pbiB3 YXMgZGV0ZWN0ZWQgYXMgdGhlIHByaW1hcnkgZWxlbWVudCBpbiB0aGUgcGFnZSwgYnV0IGlzIG5v dCB5ZXQgY3JlYXRlZC4gV2lsbCByZXN0YXJ0IGxhdGVyLiIsIHRoaXMpOworICAgICAgICAgICAg bV9kZWZlcnJlZFByb21vdGlvblRvUHJpbWFyeVBsdWdJbiA9IHRydWU7CisgICAgICAgIH0KICAg ICB9CiB9CiAKIHZvaWQgSFRNTFBsdWdJbkltYWdlRWxlbWVudDo6cmVzdGFydFNuYXBzaG90dGVk UGx1Z0luKCkKIHsKLSAgICBpZiAoZGlzcGxheVN0YXRlKCkgIT0gUmVzdGFydGluZ1dpdGhQZW5k aW5nTW91c2VDbGljaykKLSAgICAgICAgc2V0RGlzcGxheVN0YXRlKFJlc3RhcnRpbmcpOworICAg IGlmIChkaXNwbGF5U3RhdGUoKSA+PSBSZXN0YXJ0aW5nV2l0aFBlbmRpbmdNb3VzZUNsaWNrKQor ICAgICAgICByZXR1cm47CiAKKyAgICBzZXREaXNwbGF5U3RhdGUoUmVzdGFydGluZyk7CiAgICAg cmVhdHRhY2goKTsKIH0KIApAQCAtNTY1LDYgKzU3Myw4IEBAIHZvaWQgSFRNTFBsdWdJbkltYWdl RWxlbWVudDo6c3ViZnJhbWVMb2FkZXJXaWxsQ3JlYXRlUGx1Z0luKGNvbnN0IEtVUkwmIHVybCkK ICAgICBMT0coUGx1Z2lucywgIiAgIExvYWRlZCBVUkw6ICVzIiwgdXJsLnN0cmluZygpLnV0Zjgo KS5kYXRhKCkpOwogCiAgICAgbV9sb2FkZWRVcmwgPSB1cmw7CisgICAgbV9wbHVnSW5XYXNDcmVh dGVkID0gZmFsc2U7CisgICAgbV9kZWZlcnJlZFByb21vdGlvblRvUHJpbWFyeVBsdWdJbiA9IGZh bHNlOwogCiAgICAgaWYgKCFkb2N1bWVudCgpLT5wYWdlKCkgfHwgIWRvY3VtZW50KCktPnBhZ2Uo KS0+c2V0dGluZ3MoKS0+cGx1Z0luU25hcHNob3R0aW5nRW5hYmxlZCgpKSB7CiAgICAgICAgIG1f c25hcHNob3REZWNpc2lvbiA9IE5ldmVyU25hcHNob3Q7CkBAIC01ODgsNyArNTk4LDcgQEAgdm9p ZCBIVE1MUGx1Z0luSW1hZ2VFbGVtZW50OjpzdWJmcmFtZUxvYWRlcldpbGxDcmVhdGVQbHVnSW4o Y29uc3QgS1VSTCYgdXJsKQogICAgICAgICBMT0coUGx1Z2lucywgIiVwIFBsdWctaW4gaXMgYmxl c3NlZCwgYWxsb3cgaXQgdG8gc3RhcnQiLCB0aGlzKTsKICAgICAgICAgcmV0dXJuOwogICAgIH0K LSAgICAKKwogICAgIGJvb2wgaW5NYWluRnJhbWUgPSBkb2N1bWVudCgpLT5mcmFtZSgpID09IGRv Y3VtZW50KCktPnBhZ2UoKS0+bWFpbkZyYW1lKCk7CiAKICAgICBpZiAoZG9jdW1lbnQoKS0+aXNQ bHVnaW5Eb2N1bWVudCgpICYmIGluTWFpbkZyYW1lKSB7CkBAIC02NzAsMTIgKzY4MCwyMCBAQCB2 b2lkIEhUTUxQbHVnSW5JbWFnZUVsZW1lbnQ6OnN1YmZyYW1lTG9hZGVyV2lsbENyZWF0ZVBsdWdJ bihjb25zdCBLVVJMJiB1cmwpCiAKIHZvaWQgSFRNTFBsdWdJbkltYWdlRWxlbWVudDo6c3ViZnJh bWVMb2FkZXJEaWRDcmVhdGVQbHVnSW4oY29uc3QgV2lkZ2V0KiB3aWRnZXQpCiB7Ci0gICAgaWYg KCF3aWRnZXQtPmlzUGx1Z2luVmlld0Jhc2UoKSB8fCAhdG9QbHVnaW5WaWV3QmFzZSh3aWRnZXQp LT5zaG91bGRBbHdheXNBdXRvU3RhcnQoKSkKKyAgICBtX3BsdWdJbldhc0NyZWF0ZWQgPSB0cnVl OworCisgICAgaWYgKHdpZGdldC0+aXNQbHVnaW5WaWV3QmFzZSgpICYmIHRvUGx1Z2luVmlld0Jh c2Uod2lkZ2V0KS0+c2hvdWxkQWx3YXlzQXV0b1N0YXJ0KCkpIHsKKyAgICAgICAgTE9HKFBsdWdp bnMsICIlcCBQbHVnLWluIHNob3VsZCBhdXRvLXN0YXJ0LCBzZXQgdG8gcGxheSIsIHRoaXMpOwor ICAgICAgICBtX3NuYXBzaG90RGVjaXNpb24gPSBOZXZlclNuYXBzaG90OworICAgICAgICBzZXRE aXNwbGF5U3RhdGUoUGxheWluZyk7CiAgICAgICAgIHJldHVybjsKKyAgICB9CiAKLSAgICBMT0co UGx1Z2lucywgIiVwIFBsdWctaW4gc2hvdWxkIGF1dG8tc3RhcnQsIHNldCB0byBwbGF5IiwgdGhp cyk7Ci0gICAgbV9zbmFwc2hvdERlY2lzaW9uID0gTmV2ZXJTbmFwc2hvdDsKLSAgICBzZXREaXNw bGF5U3RhdGUoUGxheWluZyk7CisgICAgaWYgKG1fZGVmZXJyZWRQcm9tb3Rpb25Ub1ByaW1hcnlQ bHVnSW4pIHsKKyAgICAgICAgTE9HKFBsdWdpbnMsICIlcCBQbHVnLWluIHdhcyBjcmVhdGVkLCBw cmV2aW91c2x5IGRlZmVycmVkIHByb21vdGlvbiB0byBwcmltYXJ5LiBXaWxsIHByb21vdGUiLCB0 aGlzKTsKKyAgICAgICAgc2V0SXNQcmltYXJ5U25hcHNob3R0ZWRQbHVnSW4odHJ1ZSk7CisgICAg ICAgIG1fZGVmZXJyZWRQcm9tb3Rpb25Ub1ByaW1hcnlQbHVnSW4gPSBmYWxzZTsKKyAgICB9CiB9 CiAKIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2h0 bWwvSFRNTFBsdWdJbkltYWdlRWxlbWVudC5oIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MUGx1 Z0luSW1hZ2VFbGVtZW50LmgKaW5kZXggYjU2YzkyNy4uMjM0OWY0MiAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvaHRtbC9IVE1MUGx1Z0luSW1hZ2VFbGVtZW50LmgKKysrIGIvU291cmNlL1dl YkNvcmUvaHRtbC9IVE1MUGx1Z0luSW1hZ2VFbGVtZW50LmgKQEAgLTE2MCw2ICsxNjAsOCBAQCBw cml2YXRlOgogICAgIGJvb2wgbV9jcmVhdGVkRHVyaW5nVXNlckdlc3R1cmU7CiAgICAgYm9vbCBt X2lzUmVzdGFydGVkUGx1Z2luOwogICAgIGJvb2wgbV9uZWVkc0NoZWNrRm9yU2l6ZUNoYW5nZTsK KyAgICBib29sIG1fcGx1Z0luV2FzQ3JlYXRlZDsKKyAgICBib29sIG1fZGVmZXJyZWRQcm9tb3Rp b25Ub1ByaW1hcnlQbHVnSW47CiAgICAgSW50U2l6ZSBtX3NpemVXaGVuU25hcHNob3R0ZWQ7CiAg ICAgU25hcHNob3REZWNpc2lvbiBtX3NuYXBzaG90RGVjaXNpb247CiB9Owo=