108880 2013-02-04 17:13:40 -0800 Create a TextResourceDecoder on the BackgroundHTMLParser 2013-02-06 16:29:13 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 106127 107603 1 tonyg tonyg abarth ap ojan.autocc webkit.review.bot oldest_to_newest 824923 0 tonyg 2013-02-04 17:13:40 -0800 Create a TextResourceDecoder on the BackgroundHTMLParser 824930 1 186504 tonyg 2013-02-04 17:17:20 -0800 Created attachment 186504 Patch 825009 2 186504 abarth 2013-02-04 18:44:34 -0800 Comment on attachment 186504 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186504&action=review > Source/WebCore/html/parser/HTMLParserOptions.h:44 > + String mimeType; > + String defaultTextEncodingName; Now HTMLParserOption isn't thread-safe. Presumably we need to account for that somewhere. 825179 3 ap 2013-02-04 23:37:10 -0800 I don't think that this is safe. atomicCanonicalTextEncodingName() calls functions like buildBaseTextCodecMaps() and extendTextCodecMaps(), which are main thread only. 825279 4 186504 buildbot 2013-02-05 01:58:40 -0800 Comment on attachment 186504 Patch Attachment 186504 did not pass win-ews (win): Output: http://queues.webkit.org/results/16373569 825331 5 186504 buildbot 2013-02-05 02:58:50 -0800 Comment on attachment 186504 Patch Attachment 186504 did not pass win-ews (win): Output: http://queues.webkit.org/results/16369663 825758 6 abarth 2013-02-05 13:14:51 -0800 (In reply to comment #3) > I don't think that this is safe. atomicCanonicalTextEncodingName() calls functions like buildBaseTextCodecMaps() and extendTextCodecMaps(), which are main thread only. Is MutexLocker lock(encodingRegistryMutex()); not sufficient? 825786 7 ap 2013-02-05 13:56:52 -0800 buildBaseTextCodecMaps() has an ASSERT(isMainThread()) in it, so no, not quite sufficient. I don't remember much detail, but this code is a bit more fragile than it should be. It was particularly easy to get into a recursive deadlock by adding too many locks. We only have a mutex around codec registry - but actual codecs do interesting things threading-wise - see e.g. cachedConverterICU() in TextCodecICU.cpp. The idea here is that we have non-WebCore callers on secondary threads, but callers are tightly bound to their thread. 825879 8 abarth 2013-02-05 15:37:24 -0800 > buildBaseTextCodecMaps() has an ASSERT(isMainThread()) in it, so no, not quite sufficient. From reading the code, it seems like that ASSERT just ensures that we build the codec map on the main thread. After it is built, it looks to be useable from any thread. I haven't studied the code in detail, however. 826789 9 tonyg 2013-02-06 15:03:05 -0800 I'm not going to do this as a separate patch after all. Please see the complete thing on bug 107603. 826836 10 ap 2013-02-06 16:08:18 -0800 Can you respond to my comments? The patch in bug 107603 is large, and I'm unsure how it answers them. 826841 11 abarth 2013-02-06 16:11:11 -0800 > Can you respond to my comments? The patch in bug 107603 is large, and I'm unsure how it answers them. The patch in bug 107603 calls TextResourceDecoder::create on the main thread. 826860 12 ap 2013-02-06 16:27:20 -0800 Thank you for the explanation, that certainly resolves the concern. I misread comment 9 as implying that the same approach is part of the patch in bug 107603. 826862 13 abarth 2013-02-06 16:29:13 -0800 There's some further discussion in bug 107603 that might interest you as well. 186504 2013-02-04 17:17:20 -0800 2013-02-05 10:47:09 -0800 Patch bug-108880-20130204171359.patch text/plain 4693 tonyg U3VidmVyc2lvbiBSZXZpc2lvbjogMTQxODI0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMTRkZjJlNDAxOWI1OTdi ZTE0ZGFlMzVlNzlkMTJmMDMxODA4OTNmZi4uM2FjZDVmMTcyZjQyZDIzZDg4Y2I1NDI0ZTRlM2Iz NWJlYzIxNTVlNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDEzLTAyLTA0ICBUb255 IEdlbnRpbGNvcmUgIDx0b255Z0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgQ3JlYXRlIGEgVGV4 dFJlc291cmNlRGVjb2RlciBvbiB0aGUgQmFja2dyb3VuZEhUTUxQYXJzZXIKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwODg4MAorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIHdhbnQgdG8gY2FsbCBmaWx0 ZXJUb2tlbigpIG9uIHRoZSBiYWNrZ3JvdW5kIHRocmVhZC4gSXQgcmVxdWlyZXMgYSBUZXh0UmVz b3VyY2VEZWNvZGVyIGFyZ3VtZW50LgorICAgICAgICBUaGlzIHBhdGNoIGhlbHBzIHBhdmUgdGhl IHdheSBmb3IgY2FsbGluZyBmaWx0ZXJUb2tlbigpIGJ5IGNyZWF0aW5nIGEgVGV4dFJlc291cmNl RGVjb2RlciBvbiB0aGUgYmFja2dyb3VuZCB0aHJlYWQuCisKKyAgICAgICAgTm8gbmV3IHRlc3Rz IGJlY2F1c2Ugbm8gbmV3IGZ1bmN0aW9uYWxpdHkuCisKKyAgICAgICAgKiBodG1sL3BhcnNlci9C YWNrZ3JvdW5kSFRNTFBhcnNlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpCYWNrZ3JvdW5kSFRN TFBhcnNlcjo6QmFja2dyb3VuZEhUTUxQYXJzZXIpOgorICAgICAgICAqIGh0bWwvcGFyc2VyL0Jh Y2tncm91bmRIVE1MUGFyc2VyLmg6CisgICAgICAgIChCYWNrZ3JvdW5kSFRNTFBhcnNlcik6Cisg ICAgICAgICogaHRtbC9wYXJzZXIvSFRNTFBhcnNlck9wdGlvbnMuY3BwOgorICAgICAgICAoV2Vi Q29yZTo6SFRNTFBhcnNlck9wdGlvbnM6OkhUTUxQYXJzZXJPcHRpb25zKToKKyAgICAgICAgKiBo dG1sL3BhcnNlci9IVE1MUGFyc2VyT3B0aW9ucy5oOgorICAgICAgICAoSFRNTFBhcnNlck9wdGlv bnMpOgorCiAyMDEzLTAyLTA0ICBHdXN0YXZvIE5vcm9uaGEgU2lsdmEgIDxnbnNAZ25vbWUub3Jn PgogCiAgICAgICAgIFtTb3VwXSBSZW1vdmUgZHVwbGljYXRlIHNldHRpbmcgb2YgZmlyc3QgcGFy dHkgZm9yIGNvb2tpZXMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL0Jh Y2tncm91bmRIVE1MUGFyc2VyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL0JhY2tn cm91bmRIVE1MUGFyc2VyLmNwcAppbmRleCBiOWMyNjQ5ZGU2ODMxNjkzMjI1YzI1MWQ0NDcyYmI2 NmQwNjRmNTZjLi5kYTRlZTBlMTc0NWY2MDE2ODkyNmFhN2ExYmM4NDQ1ZTQ5NjYwY2QxIDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9CYWNrZ3JvdW5kSFRNTFBhcnNlci5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC9wYXJzZXIvQmFja2dyb3VuZEhUTUxQYXJzZXIu Y3BwCkBAIC0zNiw2ICszNiw3IEBACiAjaW5jbHVkZSAiSFRNTFRva2VuaXplci5oIgogI2luY2x1 ZGUgIk1hdGhNTE5hbWVzLmgiCiAjaW5jbHVkZSAiU1ZHTmFtZXMuaCIKKyNpbmNsdWRlICJUZXh0 UmVzb3VyY2VEZWNvZGVyLmgiCiAjaW5jbHVkZSA8d3RmL01haW5UaHJlYWQuaD4KICNpbmNsdWRl IDx3dGYvVmVjdG9yLmg+CiAjaW5jbHVkZSA8d3RmL3RleHQvVGV4dFBvc2l0aW9uLmg+CkBAIC03 OCw2ICs3OSw3IEBAIEJhY2tncm91bmRIVE1MUGFyc2VyOjpCYWNrZ3JvdW5kSFRNTFBhcnNlcihj b25zdCBIVE1MUGFyc2VyT3B0aW9ucyYgb3B0aW9ucywgY29uCiAgICAgLCBtX29wdGlvbnMob3B0 aW9ucykKICAgICAsIG1fcGFyc2VyKHBhcnNlcikKICAgICAsIG1fcGVuZGluZ1Rva2VucyhhZG9w dFB0cihuZXcgQ29tcGFjdEhUTUxUb2tlblN0cmVhbSkpCisgICAgLCBtX2RlY29kZXIoVGV4dFJl c291cmNlRGVjb2Rlcjo6Y3JlYXRlKG9wdGlvbnMubWltZVR5cGUsIG9wdGlvbnMuZGVmYXVsdFRl eHRFbmNvZGluZ05hbWUsIG9wdGlvbnMudXNlc0VuY29kaW5nRGV0ZWN0b3IpKQogewogfQogCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9CYWNrZ3JvdW5kSFRNTFBhcnNl ci5oIGIvU291cmNlL1dlYkNvcmUvaHRtbC9wYXJzZXIvQmFja2dyb3VuZEhUTUxQYXJzZXIuaApp bmRleCA2N2EyMDVhYjBmNDgzNDRkZDkzMmY2NWUyOGU5Y2Q5Mjc1MTk4OWZjLi45MTA4M2NjM2I0 ZThkNzMwNTBjNzIzOTM1ODIzOThiMzMxNGQ5ZTM1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9odG1sL3BhcnNlci9CYWNrZ3JvdW5kSFRNTFBhcnNlci5oCisrKyBiL1NvdXJjZS9XZWJDb3Jl L2h0bWwvcGFyc2VyL0JhY2tncm91bmRIVE1MUGFyc2VyLmgKQEAgLTMzLDYgKzMzLDggQEAKICNp bmNsdWRlICJIVE1MUGFyc2VyT3B0aW9ucy5oIgogI2luY2x1ZGUgIkhUTUxUb2tlbi5oIgogI2lu Y2x1ZGUgIkhUTUxUb2tlbml6ZXIuaCIKKyNpbmNsdWRlICJUZXh0UmVzb3VyY2VEZWNvZGVyLmgi CisjaW5jbHVkZSA8d3RmL1JlZlB0ci5oPgogI2luY2x1ZGUgPHd0Zi9XZWFrUHRyLmg+CiAKIG5h bWVzcGFjZSBXZWJDb3JlIHsKQEAgLTc0LDYgKzc2LDcgQEAgcHJpdmF0ZToKICAgICBIVE1MUGFy c2VyT3B0aW9ucyBtX29wdGlvbnM7CiAgICAgV2Vha1B0cjxIVE1MRG9jdW1lbnRQYXJzZXI+IG1f cGFyc2VyOwogICAgIE93blB0cjxDb21wYWN0SFRNTFRva2VuU3RyZWFtPiBtX3BlbmRpbmdUb2tl bnM7CisgICAgUmVmUHRyPFRleHRSZXNvdXJjZURlY29kZXI+IG1fZGVjb2RlcjsKIH07CiAKIGNs YXNzIFBhcnNlck1hcCB7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9I VE1MUGFyc2VyT3B0aW9ucy5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9odG1sL3BhcnNlci9IVE1MUGFy c2VyT3B0aW9ucy5jcHAKaW5kZXggMGZjNzk1MzNkMjhiMGFmYmUwMThhZTRlODFlYjViNGIzMGNk MGViNS4uNzNlOTZkNDRkOGIwOWM1MzkwZjNkMjljMDU0NGIyMGI0MDVlZDI5MiAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvaHRtbC9wYXJzZXIvSFRNTFBhcnNlck9wdGlvbnMuY3BwCisrKyBi L1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL0hUTUxQYXJzZXJPcHRpb25zLmNwcApAQCAtMjcs NiArMjcsNyBAQAogI2luY2x1ZGUgIkhUTUxQYXJzZXJPcHRpb25zLmgiCiAKICNpbmNsdWRlICJE b2N1bWVudC5oIgorI2luY2x1ZGUgIkRvY3VtZW50TG9hZGVyLmgiCiAjaW5jbHVkZSAiRnJhbWUu aCIKICNpbmNsdWRlICJTZXR0aW5ncy5oIgogCkBAIC00Nyw3ICs0OCwxMyBAQCBIVE1MUGFyc2Vy T3B0aW9uczo6SFRNTFBhcnNlck9wdGlvbnMoRG9jdW1lbnQqIGRvY3VtZW50KQogI2Vsc2UKICAg ICB1c2VUaHJlYWRpbmcgPSBmYWxzZTsKICNlbmRpZgorICAgIHVzZXNFbmNvZGluZ0RldGVjdG9y ID0gc2V0dGluZ3MgJiYgc2V0dGluZ3MtPnVzZXNFbmNvZGluZ0RldGVjdG9yKCk7CiAgICAgbWF4 aW11bURPTVRyZWVEZXB0aCA9IHNldHRpbmdzID8gc2V0dGluZ3MtPm1heGltdW1IVE1MUGFyc2Vy RE9NVHJlZURlcHRoKCkgOiBTZXR0aW5nczo6ZGVmYXVsdE1heGltdW1IVE1MUGFyc2VyRE9NVHJl ZURlcHRoOworICAgIGlmIChkb2N1bWVudCkKKyAgICAgICAgaWYgKERvY3VtZW50TG9hZGVyKiBs b2FkZXIgPSBkb2N1bWVudC0+bG9hZGVyKCkpCisgICAgICAgICAgICBtaW1lVHlwZSA9IGxvYWRl ci0+cmVzcG9uc2VNSU1FVHlwZSgpOworICAgIGlmIChzZXR0aW5ncykKKyAgICAgICAgZGVmYXVs dFRleHRFbmNvZGluZ05hbWUgPSBzZXR0aW5ncy0+ZGVmYXVsdFRleHRFbmNvZGluZ05hbWUoKTsK IH0KIAogfQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9wYXJzZXIvSFRNTFBhcnNl ck9wdGlvbnMuaCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvcGFyc2VyL0hUTUxQYXJzZXJPcHRpb25z LmgKaW5kZXggMTI1ZjNlN2NhOTY4YjMzMWE5YzczNDBlYmFmZjMyOGUxNGI0NGQ2MC4uNWY5YzA5 N2IzYTQ1ZjhjNmI0MzczOGUzZWJlYzg2MTljYjEzN2EwNCAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvaHRtbC9wYXJzZXIvSFRNTFBhcnNlck9wdGlvbnMuaAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9odG1sL3BhcnNlci9IVE1MUGFyc2VyT3B0aW9ucy5oCkBAIC0yNiw2ICsyNiw4IEBACiAjaWZu ZGVmIEhUTUxQYXJzZXJPcHRpb25zX2gKICNkZWZpbmUgSFRNTFBhcnNlck9wdGlvbnNfaAogCisj aW5jbHVkZSA8d3RmL3RleHQvV1RGU3RyaW5nLmg+CisKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAog Y2xhc3MgRG9jdW1lbnQ7CkBAIC0zNiw3ICszOCwxMCBAQCBwdWJsaWM6CiAgICAgYm9vbCBwbHVn aW5zRW5hYmxlZDsKICAgICBib29sIHVzZVByZUhUTUw1UGFyc2VyUXVpcmtzOwogICAgIGJvb2wg dXNlVGhyZWFkaW5nOworICAgIGJvb2wgdXNlc0VuY29kaW5nRGV0ZWN0b3I7CiAgICAgdW5zaWdu ZWQgbWF4aW11bURPTVRyZWVEZXB0aDsKKyAgICBTdHJpbmcgbWltZVR5cGU7CisgICAgU3RyaW5n IGRlZmF1bHRUZXh0RW5jb2RpbmdOYW1lOwogCiAgICAgZXhwbGljaXQgSFRNTFBhcnNlck9wdGlv bnMoRG9jdW1lbnQqKTsKIH07Cg==