-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds missing call of endDocument, fixes SR-2301 #2877
Conversation
Fixed: Malloc errors were unchecked. Fixed: Out of memory when creating context was ignored. Improved: Parsing from data allocated memory that was never used. Fixed: Parsing from InputStream ignored the parser return value. Fixed: Reading the BOM returned an error if the BOM is not returned in one chunck from InputStream, e.g. when InputStreams returns one byte after the other on read.
Fixed by signaling the final ("terminal") chunk to SAX.
return result | ||
} | ||
|
||
// called to start the event-driven parse. Returns YES in the event of a successful parse, and NO in case of error. | ||
open func parse() -> Bool { | ||
return parseFromStream() | ||
XMLParser.setCurrentParser(self) | ||
defer { XMLParser.setCurrentParser(nil) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I start another parser from a parser callback, will this be reentrant correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand the it: No it will not.
On the other side: That’s no difference from the current code. (old: parse → parseFromStream → setCurrentParser → parseData → callback, new: parse → setCurrentParser [→ parseFromStream] → parseData → callback)
@dhoepfl still interested in shepherding this? |
I think the reentrance problem you found is valid and should be tracked as a separate issue, because this might result in a major refactoring of XMLParser (beyond what I’m capable of doing). I will try to find time to file an issue for it tomorrow. |
Created SR-13546 regarding the reentrance problem. |
@swift-ci please rest and merge |
What a great typo. Bots deserve a rest, too. |
@swift-ci please test and merge |
Both me and the CI bot needed rest, whoops. |
SR-2301 was mostly fixed by pull request #700 (commit 30b17ed) in 2016.
The fix however still does not call the endDocument callback.
This pull request adds the necessary end of document signaling to SAX.
This pull also fixes several minor (rare) bugs like unchecked error conditions, incorrect handling of small chuncks, etc.
Please note the change in CFXMLInterface: I’m not sure if this is the best place to handle this. The interface is undocumented and returning an error that the end of the document has been reached when you send the last piece of data was unexpected. Even more so since the meaning of the return values is undocumented and the internal (SAX) errors are not mapped to the ErrorCode enum of XMLParser (Leaving this for a different fix).