85089 2012-04-27 12:50:46 -0700 [Qt] ASSERT in FontCustomPlatformDataQt.cpp with invalid font in data URI 2012-05-02 13:53:51 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 keith.rosenblatt keith.rosenblatt hausmann ostap73 pierre.rossi webkit.review.bot oldest_to_newest 611790 0 keith.rosenblatt 2012-04-27 12:50:46 -0700 FontCustomPlatformData::fontPlatformData() asserts on m_rawFont.isValid() when visiting a page with a @font-face containing a data URI src with invalid font data. 611808 1 139254 keith.rosenblatt 2012-04-27 13:08:52 -0700 Created attachment 139254 Patch 614120 2 139254 pierre.rossi 2012-05-02 02:16:30 -0700 Comment on attachment 139254 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139254&action=review Oh, and it looks like this would allow removing fast/css/font-face-download-error.html from the Skipped list, which is very nice indeed. :) > Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:77 > + delete data; Maybe we could avoid allocating and de-allocating memory for a FontCustomPlatformData instance altogether in that case by creating a temporary QRawFont instance on the stack before that and doing the load / check there. If it's valid, the implicit sharing makes copying it to the newly created FontCustomPlatformData almost free anyway. 614152 3 139254 hausmann 2012-05-02 03:28:53 -0700 Comment on attachment 139254 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139254&action=review I think the patch is correct, but at the same time I think a layout test should cover this. It might be that there is already one, in which case this should be mentioned in the ChangeLog. > Source/WebCore/ChangeLog:11 > + No new tests. Well, shouldn't there be one then? :) It doesn't sound difficult to create a layout test that triggers the assert in a debug build. >> Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:77 >> + delete data; > > Maybe we could avoid allocating and de-allocating memory for a FontCustomPlatformData instance altogether in that case by creating a temporary QRawFont instance on the stack before that and doing the load / check there. > If it's valid, the implicit sharing makes copying it to the newly created FontCustomPlatformData almost free anyway. I agree with Pierre. Either a temporary QRawFont object or putting FontCustomPlatformData into an OwnPtr that is leaked() on return. 614478 4 139847 keith.rosenblatt 2012-05-02 11:34:25 -0700 Created attachment 139847 Patch 614484 5 139254 keith.rosenblatt 2012-05-02 11:40:57 -0700 Comment on attachment 139254 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139254&action=review >> Source/WebCore/ChangeLog:11 >> + No new tests. > > Well, shouldn't there be one then? :) It doesn't sound difficult to create a layout test that triggers the assert in a debug build. Indeed there should. For some reason I thought that tests would not be run with a debug build. >>> Source/WebCore/platform/graphics/qt/FontCustomPlatformDataQt.cpp:77 >>> + delete data; >> >> Maybe we could avoid allocating and de-allocating memory for a FontCustomPlatformData instance altogether in that case by creating a temporary QRawFont instance on the stack before that and doing the load / check there. >> If it's valid, the implicit sharing makes copying it to the newly created FontCustomPlatformData almost free anyway. > > I agree with Pierre. Either a temporary QRawFont object or putting FontCustomPlatformData into an OwnPtr that is leaked() on return. Makes sense. New patch attached. Thank you for your comments. 614631 6 139847 webkit.review.bot 2012-05-02 13:53:40 -0700 Comment on attachment 139847 Patch Clearing flags on attachment: 139847 Committed r115882: <http://trac.webkit.org/changeset/115882> 614632 7 webkit.review.bot 2012-05-02 13:53:51 -0700 All reviewed patches have been landed. Closing bug. 139254 2012-04-27 13:08:52 -0700 2012-05-02 11:40:57 -0700 Patch bug-85089-20120427160850.patch text/plain 1704 keith.rosenblatt U3VidmVyc2lvbiBSZXZpc2lvbjogMTE1NDIxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNWY0ZTg1N2RhOTRhZGNh Yzk5MTJiNDRmNGE4YzA3MjFhZjRmYTlhZS4uYmNhZWI1MzUwZjgxZGFlMGYzOTY5MjZmOTg5ZDA0 ZDQzYjZiMWE3YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTA0LTI3ICBLZWl0 aCBSb3NlbmJsYXR0ICA8a2VpdGgucm9zZW5ibGF0dEBub2tpYS5jb20+CisKKyAgICAgICAgW1F0 XSBBc3NlcnQgd2l0aCBAZm9udC1mYWNlIHdpdGggaW52YWxpZCBmb250IGluIGRhdGEgVVJJCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NTA4OQorCisg ICAgICAgIERvIG5vdCByZXR1cm4gZGF0YSByZWZlcmVuY2luZyBhbiBpbnZhbGlkIFFSYXdGb250 IGZyb20gY3JlYXRlRm9udEN1c3RvbVBsYXRmb3JtRGF0YSgpLiAgSW5zdGVhZAorICAgICAgICBy ZXR1cm4gbnVsbC4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBObyBuZXcgdGVzdHMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9xdC9Gb250 Q3VzdG9tUGxhdGZvcm1EYXRhUXQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Y3JlYXRlRm9udEN1 c3RvbVBsYXRmb3JtRGF0YSk6CisKIDIwMTItMDQtMjYgIEFsZXhhbmRlciBQYXZsb3YgIDxhcGF2 bG92QGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBJbXBsZW1lbnQgdGhl ICJEaXNhYmxlIEphdmFTY3JpcHQiIG9wdGlvbiBpbiB0aGUgc2V0dGluZ3MgZGlhbG9nCmRpZmYg LS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9Gb250Q3VzdG9tUGxh dGZvcm1EYXRhUXQuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvcXQvRm9u dEN1c3RvbVBsYXRmb3JtRGF0YVF0LmNwcAppbmRleCBkZjgxZjMzYzVjZjU5YThjNDQ4YTQwZDU2 MTExMTQ5MzQ3YTg0MzNlLi5hYWY1ZGU1YWQyYzgxOWQ1MTk3YjRiNjFiOGQyYzRkY2U2ZjllYjBi IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9Gb250Q3Vz dG9tUGxhdGZvcm1EYXRhUXQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBo aWNzL3F0L0ZvbnRDdXN0b21QbGF0Zm9ybURhdGFRdC5jcHAKQEAgLTczLDYgKzczLDEwIEBAIEZv bnRDdXN0b21QbGF0Zm9ybURhdGEqIGNyZWF0ZUZvbnRDdXN0b21QbGF0Zm9ybURhdGEoU2hhcmVk QnVmZmVyKiBidWZmZXIpCiAjZWxzZQogICAgIC8vIFBpeGVsIHNpemUgZG9lc24ndCBtYXR0ZXIg YXQgdGhpcyBwb2ludCwgaXQgaXMgc2V0IGluIEZvbnRDdXN0b21QbGF0Zm9ybURhdGE6OmZvbnRQ bGF0Zm9ybURhdGEuCiAgICAgZGF0YS0+bV9yYXdGb250LmxvYWRGcm9tRGF0YShmb250RGF0YSwg LypwaXhlbFNpemUgPSAqLzAsIFFGb250OjpQcmVmZXJEZWZhdWx0SGludGluZyk7CisgICAgaWYg KCFkYXRhLT5tX3Jhd0ZvbnQuaXNWYWxpZCgpKSB7CisgICAgICAgIGRlbGV0ZSBkYXRhOworICAg ICAgICByZXR1cm4gMDsKKyAgICB9CiAjZW5kaWYKICAgICByZXR1cm4gZGF0YTsKIH0K 139847 2012-05-02 11:34:25 -0700 2012-05-02 13:53:40 -0700 Patch bug-85089-20120502143423.patch text/plain 4409 keith.rosenblatt U3VidmVyc2lvbiBSZXZpc2lvbjogMTE1ODE4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDI3N2UzZWNkMmMzZTU3 NGI3OTQ1NzgxNzcxMWQ3OWQ5MzU3YjA1NC4uMzgxMjU5NGJhNDQ1MGVmOGEzYmYxMmEyNjM4NmM0 M2ZmOGQ1YWRhMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTA1LTAyICBLZWl0 aCBSb3NlbmJsYXR0ICA8a2VpdGgucm9zZW5ibGF0dEBub2tpYS5jb20+CisKKyAgICAgICAgW1F0 XSBBU1NFUlQgaW4gRm9udEN1c3RvbVBsYXRmb3JtRGF0YVF0LmNwcCB3aXRoIGludmFsaWQgZm9u dCBpbiBkYXRhIFVSSQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9ODUwODkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBEbyBub3QgcmV0dXJuIGRhdGEgcmVmZXJlbmNpbmcgYW4gaW52YWxpZCBRUmF3Rm9udCBm cm9tIGNyZWF0ZUZvbnRDdXN0b21QbGF0Zm9ybURhdGEoKS4gIEluc3RlYWQKKyAgICAgICAgcmV0 dXJuIG51bGwuCisKKyAgICAgICAgVGVzdDogZmFzdC9jc3MvZm9udC1mYWNlLWRhdGEtdXJpLWlu dmFsaWQuaHRtbAorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvcXQvRm9udEN1c3RvbVBs YXRmb3JtRGF0YVF0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OmNyZWF0ZUZvbnRDdXN0b21QbGF0 Zm9ybURhdGEpOgorCiAyMDEyLTA1LTAyICBQYXZlbCBGZWxkbWFuICA8cGZlbGRtYW5AY2hyb21p dW0ub3JnPgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IENhbm5vdCByZWFkIHByb3BlcnR5ICds ZW5ndGgnIG9mIHVuZGVmaW5lZCBUZXh0RWRpdG9yTW9kZWwuanM6NDY3CmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9Gb250Q3VzdG9tUGxhdGZvcm1EYXRh UXQuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvcXQvRm9udEN1c3RvbVBs YXRmb3JtRGF0YVF0LmNwcAppbmRleCBkZjgxZjMzYzVjZjU5YThjNDQ4YTQwZDU2MTExMTQ5MzQ3 YTg0MzNlLi40YzViOTY0ODg1Mjg1ZWNiZTE4YmMyYzY4OTBkODQ1ZDE1Y2NiZjUzIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9Gb250Q3VzdG9tUGxhdGZv cm1EYXRhUXQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3F0L0Zv bnRDdXN0b21QbGF0Zm9ybURhdGFRdC5jcHAKQEAgLTY1LDE0ICs2NSwxOCBAQCBGb250Q3VzdG9t UGxhdGZvcm1EYXRhKiBjcmVhdGVGb250Q3VzdG9tUGxhdGZvcm1EYXRhKFNoYXJlZEJ1ZmZlciog YnVmZmVyKQogICAgIGlmIChpZCA9PSAtMSkKICAgICAgICAgcmV0dXJuIDA7CiAgICAgUV9BU1NF UlQoUUZvbnREYXRhYmFzZTo6YXBwbGljYXRpb25Gb250RmFtaWxpZXMoaWQpLnNpemUoKSA+IDAp OworI2Vsc2UKKyAgICAvLyBQaXhlbCBzaXplIGRvZXNuJ3QgbWF0dGVyIGF0IHRoaXMgcG9pbnQs IGl0IGlzIHNldCBpbiBGb250Q3VzdG9tUGxhdGZvcm1EYXRhOjpmb250UGxhdGZvcm1EYXRhLgor ICAgIFFSYXdGb250IHJhd0ZvbnQoZm9udERhdGEsIC8qcGl4ZWxTaXplID0gKi8wLCBRRm9udDo6 UHJlZmVyRGVmYXVsdEhpbnRpbmcpOworICAgIGlmICghcmF3Rm9udC5pc1ZhbGlkKCkpCisgICAg ICAgIHJldHVybiAwOwogI2VuZGlmCiAKICAgICBGb250Q3VzdG9tUGxhdGZvcm1EYXRhICpkYXRh ID0gbmV3IEZvbnRDdXN0b21QbGF0Zm9ybURhdGE7CiAjaWYgIUhBVkUoUVJBV0ZPTlQpCiAgICAg ZGF0YS0+bV9oYW5kbGUgPSBpZDsKICNlbHNlCi0gICAgLy8gUGl4ZWwgc2l6ZSBkb2Vzbid0IG1h dHRlciBhdCB0aGlzIHBvaW50LCBpdCBpcyBzZXQgaW4gRm9udEN1c3RvbVBsYXRmb3JtRGF0YTo6 Zm9udFBsYXRmb3JtRGF0YS4KLSAgICBkYXRhLT5tX3Jhd0ZvbnQubG9hZEZyb21EYXRhKGZvbnRE YXRhLCAvKnBpeGVsU2l6ZSA9ICovMCwgUUZvbnQ6OlByZWZlckRlZmF1bHRIaW50aW5nKTsKKyAg ICBkYXRhLT5tX3Jhd0ZvbnQgPSByYXdGb250OwogI2VuZGlmCiAgICAgcmV0dXJuIGRhdGE7CiB9 CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKaW5kZXggNjNhNTk5ZGM0YzY0YjA1YzMwYjQ5ZWU3MTJiNDI2Nzc1OWMwNWZhNi4uYWJhM2M3 NzBjMmJkZGE2NzBjMTFmZWJkZDM2YWIxMjlmZWEwZDNmZCAxMDA2NDQKLS0tIGEvTGF5b3V0VGVz dHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3RzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBA CisyMDEyLTA1LTAyICBLZWl0aCBSb3NlbmJsYXR0ICA8a2VpdGgucm9zZW5ibGF0dEBub2tpYS5j b20+CisKKyAgICAgICAgW1F0XSBBU1NFUlQgaW4gRm9udEN1c3RvbVBsYXRmb3JtRGF0YVF0LmNw cCB3aXRoIGludmFsaWQgZm9udCBpbiBkYXRhIFVSSQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODUwODkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICAqIGZhc3QvY3NzL2ZvbnQtZmFjZS1kYXRhLXVyaS1pbnZh bGlkLWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAgICogZmFzdC9jc3MvZm9udC1mYWNlLWRh dGEtdXJpLWludmFsaWQuaHRtbDogQWRkZWQuCisKIDIwMTItMDUtMDIgIENzYWJhIE9zenRyb2dv bsOhYyAgPG9zc3lAd2Via2l0Lm9yZz4KIAogICAgICAgICBbUXRdW01hY10gU2tpcCBuZXcgZmFp bGluZyB0ZXN0cyBhbmQgb25lIGNyYXNoaW5nIHRlc3QgdG8gcGFpbnQgdGhlIGJvdCBncmVlbi4K ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2Zhc3QvY3NzL2ZvbnQtZmFjZS1kYXRhLXVyaS1pbnZh bGlkLWV4cGVjdGVkLnR4dCBiL0xheW91dFRlc3RzL2Zhc3QvY3NzL2ZvbnQtZmFjZS1kYXRhLXVy aS1pbnZhbGlkLWV4cGVjdGVkLnR4dApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi5kNTgyMTQ3ZjU3MzAyMGNiNTA5NmFm NzRkMDM3MmU2YmVlMmIxYjMyCi0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvZmFzdC9j c3MvZm9udC1mYWNlLWRhdGEtdXJpLWludmFsaWQtZXhwZWN0ZWQudHh0CkBAIC0wLDAgKzEsMyBA QAorVGVzdCBmb3IgYnVnIDg1MDg5OiBbUXRdIEFTU0VSVCBpbiBGb250Q3VzdG9tUGxhdGZvcm1E YXRhUXQuY3BwIHdpdGggaW52YWxpZCBmb250IGluIGRhdGEgVVJJCisKK1RoaXMgdGVzdHMgZm9y IGFuIEFTU0VSVCB3aGVuIHVzaW5nIEBmb250LWZhY2Ugd2l0aCBpbnZhbGlkIGZvbnQgZGF0YQpk aWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvZmFzdC9jc3MvZm9udC1mYWNlLWRhdGEtdXJpLWludmFs aWQuaHRtbCBiL0xheW91dFRlc3RzL2Zhc3QvY3NzL2ZvbnQtZmFjZS1kYXRhLXVyaS1pbnZhbGlk Lmh0bWwKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMC4uY2FkOWY3ODUyMmI5ZjA5ODU4YjVjZTI5ZDZlYTIxMzI4MTEyNDk0 OAotLS0gL2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2Zhc3QvY3NzL2ZvbnQtZmFjZS1kYXRh LXVyaS1pbnZhbGlkLmh0bWwKQEAgLTAsMCArMSwxOCBAQAorPGh0bWw+Cis8aGVhZD4KKzxzY3Jp cHQ+CitpZiAod2luZG93LmxheW91dFRlc3RDb250cm9sbGVyKQorICAgIGxheW91dFRlc3RDb250 cm9sbGVyLmR1bXBBc1RleHQoKTsKKzwvc2NyaXB0PgorPHN0eWxlPgorQGZvbnQtZmFjZSB7Cisg ICAgZm9udC1mYW1pbHk6IGludmFsaWQtZGF0YTsKKyAgICBzcmM6dXJsKCJkYXRhOmZvbnQvdHRm O2Jhc2U2NCxWR2hwY3lCcGN5QnViM1FnWVNCbWIyNTBMaUFnVkdocGN5QnBjeUJ1YjNRZ1lTQm1i MjUwTGlBZ1ZHaHBjeUJwY3lCdWIzUWdZU0JtYjI1MExpQWdWR2hwY3lCcGN5QnViM1FnWVNCbWIy NTBMaUFnVkdocGN5QnBjeUJ1YjNRZ1lTQm1iMjUwTGdvPSIpOworfQorPC9zdHlsZT4KKzwvaGVh ZD4KKzxib2R5PgorPHA+VGVzdCBmb3IgYnVnIDxhIGhyZWY9Imh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD04NTA4OSI+ODUwODk6IFtRdF0gQVNTRVJUIGluIEZvbnRDdXN0 b21QbGF0Zm9ybURhdGFRdC5jcHAgd2l0aCBpbnZhbGlkIGZvbnQgaW4gZGF0YSBVUkk8L3A+Cis8 c3BhbiBzdHlsZT0iZm9udC1mYW1pbHk6aW52YWxpZC1kYXRhOyI+VGhpcyB0ZXN0cyBmb3IgYW4g QVNTRVJUIHdoZW4gdXNpbmcgQGZvbnQtZmFjZSB3aXRoIGludmFsaWQgZm9udCBkYXRhPC9zcGFu PgorPC9ib2R5PgorPC9odG1sPgo=