119237 2013-07-30 01:38:02 -0700 Content filter replacement data uses the encoding from the blocked page's response headers 2013-07-30 16:45:18 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 aestes aestes ap commit-queue japhet oldest_to_newest 912703 0 aestes 2013-07-30 01:38:02 -0700 Content filter replacement data uses the encoding from the blocked page's response headers 912708 1 aestes 2013-07-30 02:00:51 -0700 <rdar://problem/14374097> 912709 2 207700 aestes 2013-07-30 02:03:29 -0700 Created attachment 207700 Patch 912917 3 207700 ap 2013-07-30 10:22:02 -0700 Comment on attachment 207700 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207700&action=review > Source/WebCore/ChangeLog:15 > + Forget about encodings determined from these sources. The replacement > + data will specify an encoding in a <meta charset>, so let that be used > + instead. This doesn’t give me a good feeling. Why is charset special? What about all other HTTP header fields? Just as one example, will we still download if disposition was an attachment? > Source/WebCore/loader/DocumentLoader.h:407 > RefPtr<ContentFilter> m_contentFilter; > + bool m_loadWasBlockedByContentFiltering; Can we get this information from m_contentFilter instead of tracking it separately? 912982 4 207700 darin 2013-07-30 12:32:25 -0700 Comment on attachment 207700 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207700&action=review r=me but please consider Alexey’s question > Source/WebCore/loader/DocumentLoader.cpp:807 > + // The content filter's replacement data has a known encoding. Ignore > + // both the override encoding and the blocked response's encoding. > + if (m_loadWasBlockedByContentFiltering) { > + userChosen = false; > + encoding = String(); > + } It’s a little awkward to compute and then ignore userChosen and encoding. Would be nice to structure the code so they were not even computed. 913052 5 aestes 2013-07-30 15:28:04 -0700 (In reply to comment #3) > (From update of attachment 207700 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207700&action=review > > > Source/WebCore/ChangeLog:15 > > + Forget about encodings determined from these sources. The replacement > > + data will specify an encoding in a <meta charset>, so let that be used > > + instead. > > This doesn’t give me a good feeling. Why is charset special? What about all other HTTP header fields? Just as one example, will we still download if disposition was an attachment? I don't think charset is special. As well as the problem you mention, mitzpettel also pointed out that we parse the replacement data using the content-type of the original response (luckily we control this data). So I'm also uneasy about how we're using headers from the original response. This bug isn't trying to address these issues across the board, though. The problem of using the wrong encoding for the replacement data was encountered often enough that a targeted fix made sense to me, the other issues notwithstanding. > > > Source/WebCore/loader/DocumentLoader.h:407 > > RefPtr<ContentFilter> m_contentFilter; > > + bool m_loadWasBlockedByContentFiltering; > > Can we get this information from m_contentFilter instead of tracking it separately? Yes, good idea. I'll make the change under the auspices of Darin's review, since he asked me to consider your feedback. 913074 6 lforschler 2013-07-30 16:05:29 -0700 <rdar://problem/14374097> 913079 7 aestes 2013-07-30 16:13:21 -0700 Committed r153502: <http://trac.webkit.org/changeset/153502> 207700 2013-07-30 02:03:29 -0700 2013-07-30 12:32:25 -0700 Patch bug-119237-20130730020328.patch text/plain 4896 aestes U3VidmVyc2lvbiBSZXZpc2lvbjogMTUzNDE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODQwNjU4NmY3NjdiZjIz MWExYjU1M2Q2ZGE0ZjY1NDYxMTNlYTkwYi4uZDBmM2JkMjlkNzkxZWQ0MWU5ODQ0NjdmNjAxNDhk YjQ4ZTJlNmY4MSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDM1IEBACisyMDEzLTA3LTMwICBBbmR5 IEVzdGVzICA8YWVzdGVzQGFwcGxlLmNvbT4gIAorCisgICAgICAgIENvbnRlbnQgZmlsdGVyIHJl cGxhY2VtZW50IGRhdGEgdXNlcyB0aGUgZW5jb2RpbmcgZnJvbSB0aGUgYmxvY2tlZCBwYWdlJ3Mg cmVzcG9uc2UgaGVhZGVycworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTE5MjM3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgV2hlbiBhIGRvY3VtZW50IHNwZWNpZmllcyBhbiBlbmNvZGluZyBpbiBhbiBIVFRQ IHJlc3BvbnNlIGhlYWRlciwgb3IKKyAgICAgICAgd2hlbiB0aGUgZW1iZWRkZXIgc3BlY2lmaWVz IGFuIG92ZXJyaWRlIGVuY29kaW5nLCBhbmQgdGhlIGNvbnRlbnQgZmlsdGVyCisgICAgICAgIGJs b2NrcyB0aGUgZG9jdW1lbnQsIHdlIGludGVycHJldCB0aGUgY29udGVudCBmaWx0ZXIncyByZXBs YWNlbWVudCBkYXRhCisgICAgICAgIHVzaW5nIHRoaXMgZW5jb2RpbmcuIFRoaXMgbWlnaHQgYmUg dGhlIHdyb25nLgorCisgICAgICAgIEZvcmdldCBhYm91dCBlbmNvZGluZ3MgZGV0ZXJtaW5lZCBm cm9tIHRoZXNlIHNvdXJjZXMuIFRoZSByZXBsYWNlbWVudAorICAgICAgICBkYXRhIHdpbGwgc3Bl Y2lmeSBhbiBlbmNvZGluZyBpbiBhIDxtZXRhIGNoYXJzZXQ+LCBzbyBsZXQgdGhhdCBiZSB1c2Vk CisgICAgICAgIGluc3RlYWQuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBXZSBkb24ndCBjdXJy ZW50bHkgaGF2ZSBhIG1lY2hhbmlzbSBmb3IgdGVzdGluZyB0aGUKKyAgICAgICAgY29udGVudCBm aWx0ZXIgZnJvbSBXZWJLaXQuCisKKyAgICAgICAgKiBsb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3Bw OgorICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnRMb2FkZXI6OkRvY3VtZW50TG9hZGVyKTogSW5p dGlhbGl6ZWQKKyAgICAgICAgbV9sb2FkV2FzQmxvY2tlZEJ5Q29udGVudEZpbHRlcmluZyB0byBm YWxzZS4KKyAgICAgICAgKFdlYkNvcmU6OkRvY3VtZW50TG9hZGVyOjpmaW5pc2hlZExvYWRpbmcp OiBTZXQKKyAgICAgICAgbV9sb2FkV2FzQmxvY2tlZEJ5Q29udGVudEZpbHRlcmluZyB0byB0cnVl IGlmIHRoZSBjb250ZW50IGZpbHRlciBibG9ja2VkCisgICAgICAgIGRhdGEuCisgICAgICAgIChX ZWJDb3JlOjpEb2N1bWVudExvYWRlcjo6Y29tbWl0RGF0YSk6IFByZXRlbmQgYXMgaWYgbm8gZW5j b2RpbmcgaXMKKyAgICAgICAgc3BlY2lmaWVkIGlmIHRoZSBjb250ZW50IGZpbHRlciBibG9ja2Vk IHRoZSBsb2FkLgorICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnRMb2FkZXI6OmRhdGFSZWNlaXZl ZCk6IFN0b3BwZWQgY2FsbGluZyBjb21taXRMb2FkKCkKKyAgICAgICAgYmVmb3JlIGVhcmx5LXJl dHVybmluZyBpZiB0aGUgY29udGVudCBmaWx0ZXIgbmVlZHMgbW9yZSBkYXRhLiBUaGlzIGlzbid0 CisgICAgICAgIG5lY2Vzc2FyeS4KKyAgICAgICAgKiBsb2FkZXIvRG9jdW1lbnRMb2FkZXIuaDog RGVjbGFyZWQgbV9sb2FkV2FzQmxvY2tlZEJ5Q29udGVudEZpbHRlcmluZy4KKwogMjAxMy0wNy0y OCAgQW5keSBFc3RlcyAgPGFlc3Rlc0BhcHBsZS5jb20+CiAKICAgICAgICAgU3RvcCBleHBvcnRp bmcgV2lkZ2V0OjpmcmFtZVJlY3RzQ2hhbmdlZCgpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3Vt ZW50TG9hZGVyLmNwcAppbmRleCBlOTQwZWY1NTMxYzA0YzNmNmU5NzA5ZGFjYzJiZWY2YTg3NTdk NjM3Li40ODY3NDNmMzJiNzYyOTI1ODAzZWVlNmEyZjlhMjhjMjJlY2E0ODkxIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwCisrKyBiL1NvdXJjZS9X ZWJDb3JlL2xvYWRlci9Eb2N1bWVudExvYWRlci5jcHAKQEAgLTExOSw2ICsxMTksOSBAQCBEb2N1 bWVudExvYWRlcjo6RG9jdW1lbnRMb2FkZXIoY29uc3QgUmVzb3VyY2VSZXF1ZXN0JiByZXEsIGNv bnN0IFN1YnN0aXR1dGVEYXRhJgogICAgICwgbV9kYXRhTG9hZFRpbWVyKHRoaXMsICZEb2N1bWVu dExvYWRlcjo6aGFuZGxlU3Vic3RpdHV0ZURhdGFMb2FkTm93KQogICAgICwgbV93YWl0aW5nRm9y Q29udGVudFBvbGljeShmYWxzZSkKICAgICAsIG1fYXBwbGljYXRpb25DYWNoZUhvc3QoYWRvcHRQ dHIobmV3IEFwcGxpY2F0aW9uQ2FjaGVIb3N0KHRoaXMpKSkKKyNpZiBVU0UoQ09OVEVOVF9GSUxU RVJJTkcpCisgICAgLCBtX2xvYWRXYXNCbG9ja2VkQnlDb250ZW50RmlsdGVyaW5nKGZhbHNlKQor I2VuZGlmCiB7CiB9CiAKQEAgLTM3MSw2ICszNzQsOCBAQCB2b2lkIERvY3VtZW50TG9hZGVyOjpm aW5pc2hlZExvYWRpbmcoZG91YmxlIGZpbmlzaFRpbWUpCiAjaWYgVVNFKENPTlRFTlRfRklMVEVS SU5HKQogICAgIGlmIChtX2NvbnRlbnRGaWx0ZXIgJiYgbV9jb250ZW50RmlsdGVyLT5uZWVkc01v cmVEYXRhKCkpIHsKICAgICAgICAgbV9jb250ZW50RmlsdGVyLT5maW5pc2hlZEFkZGluZ0RhdGEo KTsKKyAgICAgICAgaWYgKG1fY29udGVudEZpbHRlci0+ZGlkQmxvY2tEYXRhKCkpCisgICAgICAg ICAgICBtX2xvYWRXYXNCbG9ja2VkQnlDb250ZW50RmlsdGVyaW5nID0gdHJ1ZTsKICAgICAgICAg aW50IGxlbmd0aDsKICAgICAgICAgY29uc3QgY2hhciogZGF0YSA9IG1fY29udGVudEZpbHRlci0+ Z2V0UmVwbGFjZW1lbnREYXRhKGxlbmd0aCk7CiAgICAgICAgIGlmIChkYXRhKQpAQCAtNzkyLDYg Kzc5NywxNiBAQCB2b2lkIERvY3VtZW50TG9hZGVyOjpjb21taXREYXRhKGNvbnN0IGNoYXIqIGJ5 dGVzLCBzaXplX3QgbGVuZ3RoKQogICAgICAgICAgICAgICAgIGVuY29kaW5nID0gbV9hcmNoaXZl LT5tYWluUmVzb3VyY2UoKS0+dGV4dEVuY29kaW5nKCk7CiAjZW5kaWYKICAgICAgICAgfQorCisj aWYgVVNFKENPTlRFTlRfRklMVEVSSU5HKQorICAgICAgICAvLyBUaGUgY29udGVudCBmaWx0ZXIn cyByZXBsYWNlbWVudCBkYXRhIGhhcyBhIGtub3duIGVuY29kaW5nLiBJZ25vcmUKKyAgICAgICAg Ly8gYm90aCB0aGUgb3ZlcnJpZGUgZW5jb2RpbmcgYW5kIHRoZSBibG9ja2VkIHJlc3BvbnNlJ3Mg ZW5jb2RpbmcuCisgICAgICAgIGlmIChtX2xvYWRXYXNCbG9ja2VkQnlDb250ZW50RmlsdGVyaW5n KSB7CisgICAgICAgICAgICB1c2VyQ2hvc2VuID0gZmFsc2U7CisgICAgICAgICAgICBlbmNvZGlu ZyA9IFN0cmluZygpOworICAgICAgICB9CisjZW5kaWYKKwogICAgICAgICBtX3dyaXRlci5zZXRF bmNvZGluZyhlbmNvZGluZywgdXNlckNob3Nlbik7CiAgICAgfQogICAgIEFTU0VSVChtX2ZyYW1l LT5kb2N1bWVudCgpLT5wYXJzaW5nKCkpOwpAQCAtODI0LDE0ICs4MzksMTUgQEAgdm9pZCBEb2N1 bWVudExvYWRlcjo6ZGF0YVJlY2VpdmVkKENhY2hlZFJlc291cmNlKiByZXNvdXJjZSwgY29uc3Qg Y2hhciogZGF0YSwgaW4KIAogICAgICAgICBpZiAobV9jb250ZW50RmlsdGVyLT5uZWVkc01vcmVE YXRhKCkpIHsKICAgICAgICAgICAgIC8vIFNpbmNlIHRoZSBmaWx0ZXIgc3RpbGwgbmVlZHMgbW9y ZSBkYXRhIHRvIG1ha2UgYSBkZWNpc2lvbiwKLSAgICAgICAgICAgIC8vIHRyYW5zaXRpb24gYmFj ayB0byB0aGUgY29tbWl0dGVkIHN0YXRlIHNvIHRoYXQgd2UgZG9uJ3QgcGFydGlhbGx5Ci0gICAg ICAgICAgICAvLyBsb2FkIGNvbnRlbnQgdGhhdCBtaWdodCBsYXRlciBiZSBibG9ja2VkLgotICAg ICAgICAgICAgY29tbWl0TG9hZCgwLCAwKTsKKyAgICAgICAgICAgIC8vIGF2b2lkIGNvbW1pdHRp bmcgdGhpcyBkYXRhIHRvIHByZXZlbnQgcGFydGlhbCByZW5kZXJpbmcgb2YKKyAgICAgICAgICAg IC8vIGNvbnRlbnQgdGhhdCBtaWdodCBsYXRlciBiZSBibG9ja2VkLgogICAgICAgICAgICAgcmV0 dXJuOwogICAgICAgICB9CiAKICAgICAgICAgZGF0YSA9IG1fY29udGVudEZpbHRlci0+Z2V0UmVw bGFjZW1lbnREYXRhKGxlbmd0aCk7CiAgICAgICAgIGxvYWRXYXNCbG9ja2VkQmVmb3JlRmluaXNo aW5nID0gbV9jb250ZW50RmlsdGVyLT5kaWRCbG9ja0RhdGEoKTsKKyAgICAgICAgaWYgKGxvYWRX YXNCbG9ja2VkQmVmb3JlRmluaXNoaW5nKQorICAgICAgICAgICAgbV9sb2FkV2FzQmxvY2tlZEJ5 Q29udGVudEZpbHRlcmluZyA9IHRydWU7CiAgICAgfQogI2VuZGlmCiAKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2xvYWRlci9Eb2N1bWVudExvYWRlci5oIGIvU291cmNlL1dlYkNvcmUvbG9h ZGVyL0RvY3VtZW50TG9hZGVyLmgKaW5kZXggNGJkMmM3YWU0NzkzNWYxYzRiNTU0MjI3YWFmZWY3 MWQxYTZkOTE2ZS4uNDg5NGJhMDhmZjliZTg3OTIwNGU4OWFlMTBhMmQxODRmZmFjNzFhOSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmgKKysrIGIvU291 cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmgKQEAgLTQwNCw2ICs0MDQsNyBAQCBu YW1lc3BhY2UgV2ViQ29yZSB7CiAKICNpZiBVU0UoQ09OVEVOVF9GSUxURVJJTkcpCiAgICAgICAg IFJlZlB0cjxDb250ZW50RmlsdGVyPiBtX2NvbnRlbnRGaWx0ZXI7CisgICAgICAgIGJvb2wgbV9s b2FkV2FzQmxvY2tlZEJ5Q29udGVudEZpbHRlcmluZzsKICNlbmRpZgogICAgIH07CiAK