85882 2012-05-08 06:47:29 -0700 [Qt] Remove ASSERT from QWebIconImageProvider::requestImage 2012-06-02 04:18:59 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 80400 1 zoltan zoltan hausmann menard rafael.lobo vestbo webkit.review.bot oldest_to_newest 618471 0 zoltan 2012-05-08 06:47:29 -0700 [Qt] Remove ASSERT from QWebIconImageProvider::requestImage 618473 1 140707 zoltan 2012-05-08 06:48:13 -0700 Created attachment 140707 Patch 618476 2 zoltan 2012-05-08 06:51:54 -0700 This assertion hits if you use MiniBrowser with my webcore imagedecoder patch from bug #80400. QtTestBrowser works well, since WK1 doesn't have this assertion. 621631 3 140707 rafael.lobo 2012-05-11 07:27:05 -0700 Comment on attachment 140707 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140707&action=review > Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:-60 > - ASSERT(!icon.isNull()); Actually the reason you're hitting this assert is not because the page doesn't have an icon but a known issue on conversion between QUrl and QString. In some cases we can't revert it back to the original value, so even after changing the icon url of a web view confirming that its icon is there, you cannot request it back at image provider level. :-/ How frequent are you hitting this assert? You should get into it only on some tricky urls with special characters. Right now, I don't think removing is a good idea. Maybe with Thiago recent QUrl changes we will have this fixed (or detect when it still need to be worked on). 626458 4 zoltan 2012-05-17 04:50:20 -0700 (In reply to comment #3) > (From update of attachment 140707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140707&action=review > > > Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:-60 > > - ASSERT(!icon.isNull()); > > Actually the reason you're hitting this assert is not because the page doesn't have an icon but a known issue on conversion between QUrl and QString. In some cases we can't revert it back to the original value, so even after changing the icon url of a web view confirming that its icon is there, you cannot request it back at image provider level. :-/ Thus you don't have a valid icon at this point (and you don't always need). What is the reason for the assert? Using a null icon is a valid state here, then why do we demand for not null icon? > How frequent are you hitting this assert? You should get into it only on some tricky urls with special characters. Right now, I don't think removing is a good idea. Maybe with Thiago recent QUrl changes we will have this fixed (or detect when it still need to be worked on). It's hitting by simple URLs frequently also, just clicking on the menus of google.com. 637081 5 vestbo 2012-05-30 03:15:33 -0700 This should be fixed now that we base64 the page-url of the image-provider url, since r118762 640000 6 140707 hausmann 2012-06-02 02:10:01 -0700 Comment on attachment 140707 Patch Bug is marked as resolved/fixed, so clearing review on this patch to take it out of the review queue. 640011 7 vestbo 2012-06-02 04:18:59 -0700 (In reply to comment #6) > (From update of attachment 140707 [details]) > Bug is marked as resolved/fixed, so clearing review on this patch to take it out of the review queue. Would be nice if the review queue was a bit smarter than that :) 140707 2012-05-08 06:48:13 -0700 2012-06-02 02:10:01 -0700 Patch bug-85882-20120508154810.patch text/plain 1755 zoltan U3VidmVyc2lvbiBSZXZpc2lvbjogMTE2MzA0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMWYyN2MwYTUyYjAyZjIw NTc4MDdjNTAzYTU3ZTQ3YzUxNzZjYTc0ZC4uNzA1NWE1MmY2NTI0MGU0Mjg0OWVmYTliM2RkYmJk Y2Q5YWU3OGIxNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEyLTA1LTA4ICBab2x0 YW4gSG9ydmF0aCAgPHpvbHRhbkB3ZWJraXQub3JnPgorCisgICAgICAgIFtRdF0gUmVtb3ZlIEFT U0VSVCBmcm9tIFFXZWJJY29uSW1hZ2VQcm92aWRlcjo6cmVxdWVzdEltYWdlCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NTg4MgorCisgICAgICAgIFJl bW92ZSBBU1NFUlQoIWljb24uaXNOdWxsKCkpIGZyb20gUVdlYkljb25JbWFnZVByb3ZpZGVyOjpy ZXF1ZXN0SW1hZ2Ugc2luY2UgCisgICAgICAgIFF0V2ViSWNvbkRhdGFiYXNlQ2xpZW50OjppY29u SW1hZ2VGb3JQYWdlVVJMIGNhbiByZXR1cm4gd2l0aCBhIG51bGwgaWNvbiBhbmQgaXQgaXMgdmFs aWQsCisgICAgICAgIHdlIGRvIHRoZSBzYW1lIGluIHRoZSBXZWJLaXQxIEFQSSdzIFFXZWJTZXR0 aW5nczo6aWNvbkZvclVybCBmdW5jdGlvbi4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvcXQvcXdlYmljb25pbWFnZXByb3Zp ZGVyLmNwcDoKKyAgICAgICAgKFFXZWJJY29uSW1hZ2VQcm92aWRlcjo6cmVxdWVzdEltYWdlKToK KwogMjAxMi0wNS0wNyAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBnbWFpbC5jb20+ CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjExNjI5OSwgcjExNjMwMSwgYW5k IHIxMTYzMDMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL3F0L3F3 ZWJpY29uaW1hZ2Vwcm92aWRlci5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL3F0 L3F3ZWJpY29uaW1hZ2Vwcm92aWRlci5jcHAKb2xkIG1vZGUgMTAwNjQ0Cm5ldyBtb2RlIDEwMDc1 NQppbmRleCBkY2U2ZWU5ZGZlY2NjYTIzMjNlMDUxMjEwZjNjYmRjNzkwN2ZkYjk0Li5kZWFjZjY1 NWNmMjc3ZjEwYmY2MWI4MTlmMTgzZjU5MDQ5MmJiNmY2Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJ UHJvY2Vzcy9BUEkvcXQvcXdlYmljb25pbWFnZXByb3ZpZGVyLmNwcAorKysgYi9Tb3VyY2UvV2Vi S2l0Mi9VSVByb2Nlc3MvQVBJL3F0L3F3ZWJpY29uaW1hZ2Vwcm92aWRlci5jcHAKQEAgLTU3LDcg KzU3LDYgQEAgUUltYWdlIFFXZWJJY29uSW1hZ2VQcm92aWRlcjo6cmVxdWVzdEltYWdlKGNvbnN0 IFFTdHJpbmcmIGlkLCBRU2l6ZSogc2l6ZSwgY29uc3QKIAogICAgIFF0V2ViSWNvbkRhdGFiYXNl Q2xpZW50KiBpY29uRGF0YWJhc2UgPSBjb250ZXh0LT5pY29uRGF0YWJhc2UoKTsKICAgICBRSW1h Z2UgaWNvbiA9IHJlcXVlc3RlZFNpemUuaXNWYWxpZCgpID8gaWNvbkRhdGFiYXNlLT5pY29uSW1h Z2VGb3JQYWdlVVJMKHBhZ2VVUkwsIHJlcXVlc3RlZFNpemUpIDogaWNvbkRhdGFiYXNlLT5pY29u SW1hZ2VGb3JQYWdlVVJMKHBhZ2VVUkwpOwotICAgIEFTU0VSVCghaWNvbi5pc051bGwoKSk7CiAK ICAgICBpZiAoc2l6ZSkKICAgICAgICAgKnNpemUgPSBpY29uLnNpemUoKTsK