44324 2010-08-20 00:35:46 -0700 [Qt] Initialize GDK before loading plugins 2011-04-19 05:15:29 -0700 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) All All RESOLVED FIXED Qt, QtTriaged P1 Normal --- 1 kling webkit-qt-unassigned adawit ademar girish hausmann kenneth vestbo webkit.review.bot oldest_to_newest 267156 0 kling 2010-08-20 00:35:46 -0700 Adobe's Flash plugin has a tendency to crash and freeze here & there. We can alleviate some of this by making sure GDK is initialized before loading the plugin. This would replace the section calling gtk_init() in PluginPackageQt. 267157 1 64931 kling 2010-08-20 00:36:58 -0700 Created attachment 64931 Proposed patch 267158 2 webkit.review.bot 2010-08-20 00:38:16 -0700 Attachment 64931 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/plugins/qt/PluginPackageQt.cpp:107: gdk_init_check is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 267159 3 girish 2010-08-20 00:46:53 -0700 LGTM other than the style fixes. 267164 4 64931 kling 2010-08-20 01:12:15 -0700 Comment on attachment 64931 Proposed patch Clearing flags on attachment: 64931 Committed r65728: <http://trac.webkit.org/changeset/65728> 267165 5 kling 2010-08-20 01:12:26 -0700 All reviewed patches have been landed. Closing bug. 267590 6 adawit 2010-08-21 12:01:55 -0700 Now I get more frequent and random flash related crashes than before. Why was the change necessary ? 267604 7 kling 2010-08-21 13:48:56 -0700 (In reply to comment #6) > Now I get more frequent and random flash related crashes than before. Why was the change necessary ? Derp! It was freezing immediately when going to e.g http://translate.google.no/ and typing something in the text box. A number of people confirmed that this fixed it for them. Problem was that we couldn't resolve "gtk_init" from the loaded plugin. The suggestion to use gdk_init() instead of gtk_init() was from Girish. 267703 8 girish 2010-08-22 11:17:51 -0700 Mmm, I don't understand how this patch solves translate.google.no freeze. The initial code invoked gtk_init. This presumably solves the problem that Flash assumes that gtk_init has already been called by the browser. The fix here was made for Flash that was loaded through nspluginwraper. nspluginwrapper does not depend on gtk or gdk and hence code that we had didn't call gtk_init. We made it now call gdk_init and some problem is solved (I don't know what exactly it solved other than translate.google.no freezing). I checked nspluginwrapper code and the thing is it does't use gtk or gdk. It loads Flash in a separate process and communicates using IPC. If anything, we should call gtk/gdk_init in that _new_ process. How does calling gtk/gdk_init in the browser help here? Obviously, it seems to help since it seems to fix the freeze but I fail to see how :) 267741 9 adawit 2010-08-22 20:48:02 -0700 (In reply to comment #8) > Mmm, I don't understand how this patch solves translate.google.no freeze. Neither do I... Specially since I tried that site with the previous fix many times and in different ways and I still was not able to get it to freeze on me. > The initial code invoked gtk_init. This presumably solves the problem that Flash assumes that gtk_init has already been called by the browser. Actually without that patch the new 10.1 series of flash player plugins from Adobe simply crash when used from a non-Gtk application. You can see the gory details in http://webkit.org/b/40567. This however is not the first time Adobe's releases have caused this particular issue. There were similar problems in some versions of of previous releases as well. That is why there are workarounds rendering engines and browsers that do not rely on Gtk, e.g. khtml & opera. > The fix here was made for Flash that was loaded through nspluginwraper. nspluginwrapper does not depend on gtk or gdk and hence code that we had > didn't call gtk_init. We made it now call gdk_init and some problem is solved (I don't know what exactly it solved other than translate.google.no > freezing). > > I checked nspluginwrapper code and the thing is it does't use gtk or gdk. It loads Flash in a separate process and communicates using IPC. If > anything, we should call gtk/gdk_init in that _new_ process. How does calling gtk/gdk_init in the browser help here? Obviously, it seems to help since > it seems to fix the freeze but I fail to see how :) Ahhh... I now see where the entire misunderstanding is coming from. You guys are looking at this issue from the prespective of using the nspluginwrapper which is completely different from using the Adobe flash player plugin directly! Obviously, in the case of the nspluginwrapper the attempt to resolve the "gtk_init" symbol from the library will fail unless of course it uses Gtk, which as you stated above is not the case. So then can we safely say that the problem with the original patch (40567) is its failure to limit the impact of the workaround to Adobe's flash player plugin only ? If that is the case, then the fix is really simple instead of what you guys did here. In fact, in addition to nspluginwrapper, this workaround should be unnecessary for other flash players like gnash and lightspark. I guess I will go ahead and open a new ticket and post my patch to address the issue discussed here. 267770 10 kling 2010-08-22 23:25:45 -0700 *** Bug 35684 has been marked as a duplicate of this bug. *** 282437 11 ademar 2010-09-21 11:06:36 -0700 Although there's some dispute on the real need of this patch, it's now part of trunk and is still marked as a candidate for webkit-2.0 (it's now too late, but anyway). I would like to cherry-pick it to QtWebkit-2.1 (the fork was made a few commits before this patch landed). Should I? Or is there a better fix/workaround available? 282438 12 kenneth 2010-09-21 11:07:54 -0700 yes please, we need all plugin fixes in 2.1 282493 13 ademar 2010-09-21 12:58:50 -0700 Revision r65728 cherry-picked into qtwebkit-2.1 with commit a74784a <http://gitorious.org/webkit/qtwebkit/commit/a74784a> 64931 2010-08-20 00:36:58 -0700 2010-08-20 01:12:15 -0700 Proposed patch bug-44324.diff text/plain 3432 kling ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MjE4ZGQ3YS4uNjc5Y2M1ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAxMC0wOC0yMCAgQW5kcmVhcyBLbGlu ZyAgPGFuZHJlYXMua2xpbmdAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFtRdF0gSW5pdGlhbGl6ZSBHREsgYmVmb3JlIGxvYWRpbmcg cGx1Z2lucworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 NDQzMjQKKworICAgICAgICBBdHRlbXB0IHRvIGNhbGwgZ2RrX2luaXRfY2hlY2soKSBiZWZvcmUg bG9hZGluZyBhbnkgcGx1Z2lucy4KKyAgICAgICAgVGhpcyBwcmV2ZW50cyB2YXJpb3VzIGNyYXNo ZXMgYW5kIGZyZWV6ZXMgaW4gQWRvYmUncyBGbGFzaCBwbHVnaW4uCisKKyAgICAgICAgKiBwbHVn aW5zL3F0L1BsdWdpblBhY2thZ2VRdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjppbml0aWFsaXpl R2RrSWZQb3NzaWJsZSk6CisgICAgICAgIChXZWJDb3JlOjpQbHVnaW5QYWNrYWdlOjpsb2FkKToK KwogMjAxMC0wOC0xOSAgVmluY2VudCBTY2hlaWIgIDxzY2hlaWJAY2hyb21pdW0ub3JnPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IERhdmlkIExldmluLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9wbHVn aW5zL3F0L1BsdWdpblBhY2thZ2VRdC5jcHAgYi9XZWJDb3JlL3BsdWdpbnMvcXQvUGx1Z2luUGFj a2FnZVF0LmNwcAppbmRleCAwNzE0OWYzLi5iNDIxYjFlIDEwMDY0NAotLS0gYS9XZWJDb3JlL3Bs dWdpbnMvcXQvUGx1Z2luUGFja2FnZVF0LmNwcAorKysgYi9XZWJDb3JlL3BsdWdpbnMvcXQvUGx1 Z2luUGFja2FnZVF0LmNwcApAQCAtMzUsOCArMzUsNiBAQAogCiBuYW1lc3BhY2UgV2ViQ29yZSB7 CiAKLXR5cGVkZWYgdm9pZCBndGtJbml0RnVuYyhpbnQgKmFyZ2MsIGNoYXIgKioqYXJndik7Ci0K IGJvb2wgUGx1Z2luUGFja2FnZTo6ZmV0Y2hJbmZvKCkKIHsKICAgICBpZiAoIWxvYWQoKSkKQEAg LTkyLDYgKzkwLDI4IEBAIHN0YXRpYyBOUEVycm9yIHN0YXRpY1BsdWdpblF1aXJrUmVxdWlyZXNH dGtUb29sS2l0X05QTl9HZXRWYWx1ZShOUFAgaW5zdGFuY2UsIE5QCiAgICAgcmV0dXJuIE5QTl9H ZXRWYWx1ZShpbnN0YW5jZSwgdmFyaWFibGUsIHZhbHVlKTsKIH0KIAorc3RhdGljIHZvaWQgaW5p dGlhbGl6ZUdka0lmUG9zc2libGUoKQoreworICAgIHN0YXRpYyBib29sIGF0dGVtcHRNYWRlID0g ZmFsc2U7CisKKyAgICBpZiAoYXR0ZW1wdE1hZGUpCisgICAgICAgIHJldHVybjsKKworICAgIGF0 dGVtcHRNYWRlID0gdHJ1ZTsKKworICAgIFFMaWJyYXJ5IGxpYnJhcnkoImxpYmdkay14MTEtMi4w LnNvLjAiKTsKKyAgICBpZiAoIWxpYnJhcnkubG9hZCgpKQorICAgICAgICByZXR1cm47CisKKyAg ICB0eXBlZGVmIHZvaWQgKigqZ2RrX2luaXRfY2hlY2tfcHRyKShpbnQqLCBjaGFyKioqKTsKKyAg ICBnZGtfaW5pdF9jaGVja19wdHIgZ2RrX2luaXRfY2hlY2sgPSAoZ2RrX2luaXRfY2hlY2tfcHRy KWxpYnJhcnkucmVzb2x2ZSgiZ2RrX2luaXRfY2hlY2siKTsKKyAgICBpZiAoIWdka19pbml0X2No ZWNrKQorICAgICAgICByZXR1cm47CisKKyAgICAvLyBOT1RFOiBXZSdyZSB1c2luZyBnZGtfaW5p dF9jaGVjaygpIHNpbmNlIGdka19pbml0KCkgbWF5IGV4aXQoKSBvbiBmYWlsdXJlLgorICAgICh2 b2lkKSBnZGtfaW5pdF9jaGVjaygwLCAwKTsKK30KKwogYm9vbCBQbHVnaW5QYWNrYWdlOjpsb2Fk KCkKIHsKICAgICBpZiAobV9pc0xvYWRlZCkgewpAQCAtMTExLDcgKzEzMSw2IEBAIGJvb2wgUGx1 Z2luUGFja2FnZTo6bG9hZCgpCiAKICAgICBOUF9Jbml0aWFsaXplRnVuY1B0ciBOUF9Jbml0aWFs aXplOwogICAgIE5QRXJyb3IgbnBFcnI7Ci0gICAgZ3RrSW5pdEZ1bmMqIGd0a0luaXQ7CiAKICAg ICBOUF9Jbml0aWFsaXplID0gKE5QX0luaXRpYWxpemVGdW5jUHRyKW1fbW9kdWxlLT5yZXNvbHZl KCJOUF9Jbml0aWFsaXplIik7CiAgICAgbV9OUFBfU2h1dGRvd24gPSAoTlBQX1NodXRkb3duUHJv Y1B0ciltX21vZHVsZS0+cmVzb2x2ZSgiTlBfU2h1dGRvd24iKTsKQEAgLTEzMCwyNSArMTQ5LDgg QEAgYm9vbCBQbHVnaW5QYWNrYWdlOjpsb2FkKCkKICAgICAgICAgbV9icm93c2VyRnVuY3MuZ2V0 dmFsdWUgPSBzdGF0aWNQbHVnaW5RdWlya1JlcXVpcmVzR3RrVG9vbEtpdF9OUE5fR2V0VmFsdWU7 CiAgICAgfQogCi0gICAgLy8gV09SS0FST1VORDogUHJldmVudCBndGsgYmFzZWQgcGx1Z2luIGNy YXNoZXMgc3VjaCBhcyBCUiMgNDA1NjcgYnkKLSAgICAvLyBleHBsaWNpdGx5IGZvcmNpbmcgdGhl IGluaXRpYWxpemluZyBvZiBHdGssIGkuZS4gY2FsbGluZyBndGtfaW5pdCwKLSAgICAvLyB3aGVu dmVyIHRoZSBzeW1ib2wgaXMgcHJlc2VudCBpbiB0aGUgcGx1Z2luIGxpYnJhcnkgbG9hZGVkIGFi b3ZlLgotICAgIC8vIE5vdGUgdGhhdCB0aGlzIHdvcmthcm91bmQgaXMgYmFzZWQgb24gY29kZSBm cm9tIHRoZSBOU1BsdWdpbkNsYXNzIGN0b3IKLSAgICAvLyBpbiBLREUncyBrZGViYXNlL2FwcHMv bnNwbHVnaW5zL3ZpZXdlci9uc3BsdWdpbi5jcHAgZmlsZS4KLSAgICBndGtJbml0ID0gKGd0a0lu aXRGdW5jKiltX21vZHVsZS0+cmVzb2x2ZSgiZ3RrX2luaXQiKTsKLSAgICBpZiAoZ3RrSW5pdCkg ewotICAgICAgICAvLyBQcmV2ZW50IGd0a19pbml0KCkgZnJvbSByZXBsYWNpbmcgdGhlIFggZXJy b3IgaGFuZGxlcnMsIHNpbmNlIHRoZSBHdGsKLSAgICAgICAgLy8gaGFuZGxlcnMgYWJvcnQgd2hl biB0aGV5IHJlY2VpdmUgYW4gWCBlcnJvciwgdGh1cyBraWxsaW5nIHRoZSB2aWV3ZXIuCi0jaWZk ZWYgUV9XU19YMTEKLSAgICAgICAgaW50ICgqb2xkX2Vycm9yX2hhbmRsZXIpKERpc3BsYXkqLCBY RXJyb3JFdmVudCopID0gWFNldEVycm9ySGFuZGxlcigwKTsKLSAgICAgICAgaW50ICgqb2xkX2lv X2Vycm9yX2hhbmRsZXIpKERpc3BsYXkqKSA9IFhTZXRJT0Vycm9ySGFuZGxlcigwKTsKLSNlbmRp ZgotICAgICAgICBndGtJbml0KDAsIDApOwotI2lmZGVmIFFfV1NfWDExCi0gICAgICAgIFhTZXRF cnJvckhhbmRsZXIob2xkX2Vycm9yX2hhbmRsZXIpOwotICAgICAgICBYU2V0SU9FcnJvckhhbmRs ZXIob2xkX2lvX2Vycm9yX2hhbmRsZXIpOwotI2VuZGlmCi0gICAgfQorICAgIC8vIFRyeSB0byBp bml0aWFsaXplIEdESyAtIHNvbWUgdmVyc2lvbnMgb2YgdGhlIEZsYXNoIHBsdWdpbiBkZXBlbmQg b24gdGhpcy4KKyAgICBpbml0aWFsaXplR2RrSWZQb3NzaWJsZSgpOwogCiAjaWYgZGVmaW5lZChY UF9VTklYKQogICAgIG5wRXJyID0gTlBfSW5pdGlhbGl6ZSgmbV9icm93c2VyRnVuY3MsICZtX3Bs dWdpbkZ1bmNzKTsK