NullPointerException in XmlSaxPushParser#native_write triggered by fix for #1261 #1710
Labels
Comments
since you're catching the NPE with the ideally - just follow the usual way - setup a reproducer within Nokogiri's tests where one can observe the NPE, from there one can reason whether catching all exceptions is legit (or maybe catching NPE explicitly with an explanation works better). having a stack trace around here might have been useful as well (if for nothing else - at least for search engines to index and guide additional traffic here). |
I thought it was strange because the I would have produced a test case but Blather is quite complex and I could not determine what these 15 test cases had in common that caused it do this. I can provide a stack trace though. This is against d35ed46. |
|
Opened #1717 to address the NPE in this issue as well as other differences between JRuby and MRI.
|
|
Thanks @jvshahid but unfortunately your changes only fix 3 of the 15 failures in Blather and the error message has now changed. |
|
it doesn't look like you are using the right branch. in fact, the NPE in populateCharacters was what i was seeing yesterday. i am also having some trouble running blather's test suite. there are some EventMachine errors about an undefined `stopping?' method. i am also seeing a bunch of errors in client_spec which could be a result of the patch. i don't remember seeing them yesterday, but i could have accidentally introduced something while rebasing the commit and splitting it up.
i think at this point it would be useful if you can provide some minimal test cases that demonstrate the differences between MRI and JRuby.
|
|
Pretty sure it's the right branch at commit 090563c and with your other fix for #1708 cherry-picked on top (otherwise I'd get a lot more errors). Blather master is broken in other ways and I have fixes locally, the main one being that EventMachine needs to be pinned back to |
|
For now, I've just pushed up what I have to my new-nokogiri branch. Hope that helps. |
|
@flavorjones, this isn't resolved yet. While #1717 seemed to help a little, I am still seeing test failures. |
|
Just ran the Blather tests against master to double check. Definitely still an issue. |
|
I haven't ran the Blather tests in a while. Out of curiosity how many failures are you getting now ? |
|
The same 12 I mentioned previously so you have fixed 3 since I originally reported this. If you use my new-nokogiri branch then you should be able to run the suite without the problems you experienced before. |
|
I thought I should check whether #1721 helps now that it's been merged as it changes the code mentioned above. The failures remain and it also breaks my suggested fixes. It now gets |
|
@kares do you have some time to dig in on this issue? |
|
I think I got it! It's hard to explain how I got here (fun with EventMachine) but this fixes everything: diff --git a/ext/java/nokogiri/internals/NokogiriHandler.java b/ext/java/nokogiri/internals/NokogiriHandler.java
index 1969a604..35b7d597 100644
--- a/ext/java/nokogiri/internals/NokogiriHandler.java
+++ b/ext/java/nokogiri/internals/NokogiriHandler.java
@@ -322,7 +322,7 @@ public class NokogiriHandler extends DefaultHandler2 implements XmlDeclHandler {
}
protected void populateCharacters() {
- if (charactersBuilder.length() > 0) {
+ if (charactersBuilder != null && charactersBuilder.length() > 0) {
call("characters", runtime.newString(charactersBuilder.toString()));
charactersBuilder.setLength(0);
}Should it ever be in this state to begin with? I don't know but hopefully you can reason it through. |
|
@jvshahid any thoughts? |
|
That makes sense since |
|
I was able to reproduce this with empty documents, i.e. |
jvshahid
added a commit
that referenced
this issue
Mar 29, 2018
|
opened #1748 |
|
@chewi out of curiosity do you see this error message on MRI |
|
Pretty sure everything has been passing for ages on MRI but I'll double check against master tomorrow. Many thanks again for your help. |
|
Tested MRI and don't see that error. I can see in |
|
You are indeed correct. Not being the original author, I unfortunately didn't spot that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The last remaining 15 Blather test failures are caused @yokolet's fix for #1261 in 3b121ca. I'm not sure exactly what triggers it but sometimes
parserTask.parser.getNokogiriHandler()returnsnull, leading to a NullPointerException.Reverting the change allows all the Blather tests to pass. So does skipping the
tryblock if the handler isnull. Strangely changingSAXExceptionto justExceptionin thecatchclause also makes it work. I'm not sure what the correct fix would be, though presumably reverting the change isn't the way to go.The text was updated successfully, but these errors were encountered: