17353 2008-02-13 17:20:41 -0800 XMLTokenizer installs global libxml2 callbacks that can break client applications 2016-02-01 15:43:52 -0800 1 1 1 Unclassified WebKit XML 528+ (Nightly build) All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=153683 Gtk P1 Normal --- 1 alp webkit-unassigned ap mrowe sam oldest_to_newest 70586 0 alp 2008-02-13 17:20:41 -0800 The developers of Yelp (the GNOME integrated help system) found that the XML parsing functionality of their application started to fail after they replaced Mozilla with a WebKit GTK+ WebView widget. Their WebView code is completely separate from the XML parsing code (which uses libxml2), but both run in the same process. I did some debugging and tracked down the issue. XMLTokenizer.cpp assigns global match/open/read/write/close functions. By modifying the behaviour of these functions, WebKit cripples XML features throughout the embedding application: static xmlParserCtxtPtr createStringParser(xmlSAXHandlerPtr handlers, void* userData) { static bool didInit = false; if (!didInit) { xmlInitParser(); xmlRegisterInputCallbacks(matchFunc, openFunc, readFunc, closeFunc); xmlRegisterOutputCallbacks(matchFunc, openFunc, writeFunc, closeFunc); didInit = true; } ... I'm not familiar with XMLTokenizer.cpp so not sure how to proceed. Any thoughts? 70597 1 mrowe 2008-02-13 22:15:30 -0800 The IO callbacks registered by xmlRegisterInputCallbacks/xmlRegisterOutputCallbacks go onto a global stack of callbacks that are asked in turn whether they can handle the IO for a given URL. The fact that these can only be set globally looks to be a limitation of libxml2. The fact that the callbacks are only provided with the URL when asked whether they can handle the load makes it challenging to have WebCore's callbacks limit themselves to handling IO for WebCore-related resources. Not using WebCore's callbacks when globalDocLoader may reduce the problem a little, but is unlikely to be safe in a multithreaded application. What is the failure mode that is occurring in applications? My reading of the code suggests that documents may be returned as zero-length if there is no associated WebCore loader, which I would imagine to be the case for uses of libxml2 outside WebCore. 70648 2 alp 2008-02-14 04:59:01 -0800 Original bug report: Hi, (Feel free to forward this to anyone you feel like / might be more appropriate) As you may have seen on planet.gnome.org [1]), I'm trying to get yelp running with webkit. I'm hitting some issues working with libxml / libxslt and webkit_web_view_load_string which I'm using to load local files. The problem manifests when I set the mime type to "application/xhtml +xml", which is required for all our documents. The problem is that when I set the mime type to application/xhtml+xml, any subsequent calls to xslt will fail as libxml thinks the document is empty. I'm not entirely certain why it thinks this, so I thought I'd get some expert opinion ;) I've produced a test program, based on the example browser and which can be found at: http://www.gnome.org/~dscorgie/main.c Vague instructions. Compile using: gcc -o main main.c `pkg-config --cflags --libs WebKitGtk libxslt` then run with: ./main text/html to run the program working. When you do this, click the link and press back. You should see output in the terminal like: ss1: 0x807d690 ss2: 0x82261e8 which means libxslt has successfully parsed the requested stylesheet. The output are 2 pointers to stylesheets, the first (ss1) is generated just before running the web_view and the second (ss2) generated in the "go_back" callback (for lack of a better place). To change the mime type to xhtml, you can omit the argument: ./main which should produce the dreaded output (after clicking the link and pressing back): ss1: 0x807d690 /usr/share/yelp/xslt/yelp-common.xsl:1: parser error : Document is empty ^ /usr/share/yelp/xslt/yelp-common.xsl:1: parser error : Start tag expected, '<' not found ^ error xsltParseStylesheetFile : cannot parse /usr/share/yelp/xslt/yelp-common.xsl ss2: (nil) which means the program now thinks /usr/share/yelp/xslt/yelp-common.xsl is empty. I feel I'm doing something stupid, but have been staring at this (and tracing through libxml, libxslt and yelp) for 2 weeks and still can't see it. Oh, I should also mention you need yelp installed on you're system (or change the example to point to a different stylesheet in both places). Also, this is with nightly builds r29907 and r30123 (the latest). Thanks Don [1] progress can be tracked in the bug: http://bugzilla.gnome.org/show_bug.cgi?id=512827 70692 3 19125 alp 2008-02-14 13:44:44 -0800 Created attachment 19125 Proposed fix This fixes the reported breakage in applications. 70693 4 19125 darin 2008-02-14 13:50:24 -0800 Comment on attachment 19125 Proposed fix r=me, with some reservations. Doesn't this still leave us with the problem that other libxml2 clients could call xmlRegisterInputCallbacks and clobber our callbacks with their own? Is there a "save and restore" approach we could use instead? Should we file a bug asking libxml2 to come up with a way of doing this that's not global? + // TODO: We should restore the original global error handler as well. We use FIXME, not TODO, for these things. 70694 5 alp 2008-02-14 13:51:49 -0800 (In reply to comment #3) > Created an attachment (id=19125) [edit] > Proposed fix > > This fixes the reported breakage in applications. > The patch deals with all potential re-entrancy I can think of but any thoughts here are welcome. It might be worth filing a follow-up bug to track/audit other issues like this in WebCore. The failure mode is very obscure and we're quite lucky this bug was reported to us. 70695 6 alp 2008-02-14 13:57:37 -0800 (In reply to comment #4) > (From update of attachment 19125 [edit]) > r=me, with some reservations. > > Doesn't this still leave us with the problem that other libxml2 clients could > call xmlRegisterInputCallbacks and clobber our callbacks with their own? Is > there a "save and restore" approach we could use instead? Yep, totally. It's not a complete fix. Luckily very few applications seem to use xmlRegisterInputCallbacks(), so this saves our skin for now. I tried cooking up a save/restore form of this patch but it still didn't address the reverse scenario you described. Could you elaborate or maybe give a proof of concept? > > Should we file a bug asking libxml2 to come up with a way of doing this that's > not global? I think dv is aware that the globals are bad. It's possible they've added new a new way to do this and we haven't seen it yet. We should file a bug or check up on the existing one (Eric believes a bug has already been filed). 70713 7 19125 alp 2008-02-14 16:33:29 -0800 Comment on attachment 19125 Proposed fix Patch landed in r30236. Will keep the bug open at least for a few days to see how things pan out. Maybe we can come up with a more complete solution. 70753 8 alp 2008-02-15 05:18:23 -0800 Guess we can close this. 19125 2008-02-14 13:44:44 -0800 2008-02-14 16:33:29 -0800 Proposed fix thr4.patch text/plain 3485 alp ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MzJmZGI5Mi4uMzZlZDVmMyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyNSBAQAorMjAwOC0wMi0xNCAgQWxwIFRva2VyICA8 YWxwQGF0b2tlci5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgaHR0cDovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTczNTMKKyAg ICAgICAgWE1MVG9rZW5pemVyIGluc3RhbGxzIGdsb2JhbCBsaWJ4bWwyIGNhbGxiYWNrcyB0aGF0 IGNhbiBicmVhayBjbGllbnQgYXBwbGljYXRpb25zCisKKyAgICAgICAgUGF0Y2ggYnkgTWFyayBS b3dlICh3aXRoIGEgZmV3IGNoYW5nZXMpLgorCisgICAgICAgIFRoZSB4bWxSZWdpc3RlcklucHV0 Q2FsbGJhY2tzL3htbFJlZ2lzdGVyT3V0cHV0Q2FsbGJhY2tzIGRvbmUgYXQKKyAgICAgICAgaW5p dCBhcmUgZ2xvYmFsIHNvIHdlIG5lZWQgdG8gbWFrZSBzdXJlIHRoZXNlIGNhbGxiYWNrcyBvbmx5 IGdldCB1c2VkCisgICAgICAgIGJ5IFhNTFRva2VuaXplciBhbmQgbmV2ZXIgYnkgbGlieG1sMiBj YWxscyBpbiB1c2VyIGFwcGxpY2F0aW9ucy4KKworICAgICAgICBUaGlzIHBhdGNoIG1vZGlmaWVz IHRoZSBtYXRjaCBhbmQgb3BlbiBmdW5jdGlvbnMgdG8gb25seSBhcHBseSB3aGVuIHdlCisgICAg ICAgIGFyZSBjZXJ0YWluIHRoZSBjYWxsZXIgaXMgWE1MVG9rZW5pemVyIGJ5IGNoZWNraW5nIGds b2JhbERvY0xvYWRlciBhbmQKKyAgICAgICAgZW5zdXJpbmcgd2UncmUgb24gdGhlIGNvcnJlY3Qg dGhyZWFkLgorCisgICAgICAgICogZG9tL1hNTFRva2VuaXplci5jcHA6CisgICAgICAgIChXZWJD b3JlOjptYXRjaEZ1bmMpOgorICAgICAgICAoV2ViQ29yZTo6b3BlbkZ1bmMpOgorICAgICAgICAo V2ViQ29yZTo6Y3JlYXRlU3RyaW5nUGFyc2VyKToKKwogMjAwOC0wMi0xMyAgSnVzdGluIEdhcmNp YSAgPGp1c3Rpbi5nYXJjaWFAYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IE9saXZl ciBIdW50LgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9kb20vWE1MVG9rZW5pemVyLmNwcCBiL1dlYkNv cmUvZG9tL1hNTFRva2VuaXplci5jcHAKaW5kZXggN2JhNDI0Yy4uYTNiMzRmMSAxMDA2NDQKLS0t IGEvV2ViQ29yZS9kb20vWE1MVG9rZW5pemVyLmNwcAorKysgYi9XZWJDb3JlL2RvbS9YTUxUb2tl bml6ZXIuY3BwCkBAIC00Nyw2ICs0Nyw3IEBACiAjaW5jbHVkZSAiUmVzb3VyY2VIYW5kbGUuaCIK ICNpbmNsdWRlICJSZXNvdXJjZVJlcXVlc3QuaCIKICNpbmNsdWRlICJSZXNvdXJjZVJlc3BvbnNl LmgiCisjaW5jbHVkZSAiVGhyZWFkaW5nLmgiCiAjaWZuZGVmIFVTRV9RWE1MU1RSRUFNCiAjaW5j bHVkZSA8bGlieG1sL3BhcnNlci5oPgogI2luY2x1ZGUgPGxpYnhtbC9wYXJzZXJJbnRlcm5hbHMu aD4KQEAgLTM0MCwxNCArMzQxLDE2IEBAIHB1YmxpYzoKIC8vIC0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tCiAKIHN0YXRpYyBpbnQgZ2xvYmFsRGVzY3JpcHRvciA9IDA7CitzdGF0aWMg RG9jTG9hZGVyKiBnbG9iYWxEb2NMb2FkZXIgPSAwOworc3RhdGljIFRocmVhZElkZW50aWZpZXIg bGlieG1sTG9hZGVyVGhyZWFkID0gMDsKIAogc3RhdGljIGludCBtYXRjaEZ1bmMoY29uc3QgY2hh ciogdXJpKQogewotICAgIHJldHVybiAxOyAvLyBNYXRjaCBldmVyeXRoaW5nLgorICAgIC8vIE9u bHkgbWF0Y2ggbG9hZHMgaW5pdGlhdGVkIGR1ZSB0byB1c2VzIG9mIGxpYnhtbDIgZnJvbSB3aXRo aW4gWE1MVG9rZW5pemVyIHRvIGF2b2lkCisgICAgLy8gaW50ZXJmZXJpbmcgd2l0aCBjbGllbnQg YXBwbGljYXRpb25zIHRoYXQgYWxzbyB1c2UgbGlieG1sMi4gIGh0dHA6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE3MzUzCisgICAgcmV0dXJuIGdsb2JhbERvY0xvYWRlciAmJiBj dXJyZW50VGhyZWFkKCkgPT0gbGlieG1sTG9hZGVyVGhyZWFkOwogfQogCi1zdGF0aWMgRG9jTG9h ZGVyKiBnbG9iYWxEb2NMb2FkZXIgPSAwOwotCiBjbGFzcyBPZmZzZXRCdWZmZXIgewogcHVibGlj OgogICAgIE9mZnNldEJ1ZmZlcihjb25zdCBWZWN0b3I8Y2hhcj4mIGIpIDogbV9idWZmZXIoYiks IG1fY3VycmVudE9mZnNldCgwKSB7IH0KQEAgLTM3NywxNSArMzgwLDI0IEBAIHN0YXRpYyBib29s IHNob3VsZEFsbG93RXh0ZXJuYWxMb2FkKGNvbnN0IGNoYXIqIGluVVJJKQogfQogc3RhdGljIHZv aWQqIG9wZW5GdW5jKGNvbnN0IGNoYXIqIHVyaSkKIHsKLSAgICBpZiAoIWdsb2JhbERvY0xvYWRl ciB8fCAhc2hvdWxkQWxsb3dFeHRlcm5hbExvYWQodXJpKSkKKyAgICBBU1NFUlQoZ2xvYmFsRG9j TG9hZGVyKTsKKyAgICBBU1NFUlQoY3VycmVudFRocmVhZCgpID09IGxpYnhtbExvYWRlclRocmVh ZCk7CisKKyAgICBpZiAoIXNob3VsZEFsbG93RXh0ZXJuYWxMb2FkKHVyaSkpCiAgICAgICAgIHJl dHVybiAmZ2xvYmFsRGVzY3JpcHRvcjsKIAogICAgIFJlc291cmNlRXJyb3IgZXJyb3I7CiAgICAg UmVzb3VyY2VSZXNwb25zZSByZXNwb25zZTsKICAgICBWZWN0b3I8Y2hhcj4gZGF0YTsKICAgICAK LSAgICBpZiAoZ2xvYmFsRG9jTG9hZGVyLT5mcmFtZSgpKSAKLSAgICAgICAgZ2xvYmFsRG9jTG9h ZGVyLT5mcmFtZSgpLT5sb2FkZXIoKS0+bG9hZFJlc291cmNlU3luY2hyb25vdXNseShLVVJMKHVy aSksIGVycm9yLCByZXNwb25zZSwgZGF0YSk7CisgICAgRG9jTG9hZGVyKiBkb2NMb2FkZXIgPSBn bG9iYWxEb2NMb2FkZXI7CisgICAgZ2xvYmFsRG9jTG9hZGVyID0gMDsKKyAgICAvLyBUT0RPOiBX ZSBzaG91bGQgcmVzdG9yZSB0aGUgb3JpZ2luYWwgZ2xvYmFsIGVycm9yIGhhbmRsZXIgYXMgd2Vs bC4KKworICAgIGlmIChkb2NMb2FkZXItPmZyYW1lKCkpIAorICAgICAgICBkb2NMb2FkZXItPmZy YW1lKCktPmxvYWRlcigpLT5sb2FkUmVzb3VyY2VTeW5jaHJvbm91c2x5KEtVUkwodXJpKSwgZXJy b3IsIHJlc3BvbnNlLCBkYXRhKTsKKworICAgIGdsb2JhbERvY0xvYWRlciA9IGRvY0xvYWRlcjsK IAogICAgIHJldHVybiBuZXcgT2Zmc2V0QnVmZmVyKGRhdGEpOwogfQpAQCAtNDMyLDYgKzQ0NCw3 IEBAIHN0YXRpYyB4bWxQYXJzZXJDdHh0UHRyIGNyZWF0ZVN0cmluZ1BhcnNlcih4bWxTQVhIYW5k bGVyUHRyIGhhbmRsZXJzLCB2b2lkKiB1c2VyCiAgICAgICAgIHhtbEluaXRQYXJzZXIoKTsKICAg ICAgICAgeG1sUmVnaXN0ZXJJbnB1dENhbGxiYWNrcyhtYXRjaEZ1bmMsIG9wZW5GdW5jLCByZWFk RnVuYywgY2xvc2VGdW5jKTsKICAgICAgICAgeG1sUmVnaXN0ZXJPdXRwdXRDYWxsYmFja3MobWF0 Y2hGdW5jLCBvcGVuRnVuYywgd3JpdGVGdW5jLCBjbG9zZUZ1bmMpOworICAgICAgICBsaWJ4bWxM b2FkZXJUaHJlYWQgPSBjdXJyZW50VGhyZWFkKCk7CiAgICAgICAgIGRpZEluaXQgPSB0cnVlOwog ICAgIH0KIAo=