-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
new XMLSerializer().serializeToString(document); panics #23130
Comments
As @jdm mentioned in #23044 (comment) , I think this will be fixed when servo/html5ever#368 is fixed. |
Specifically, this would be handled by https://w3c.github.io/DOM-Parsing/#dfn-xml-serialization-algorithm. |
Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the If you intend to work on this issue, then add |
That being said, fixing the panic here is separate from implementing the full algorithm in the html5ever repository. We should implement the document serialization algorithm by checking for a document in the SerializationIterator constructor and pushing all of the immediate child nodes of the document. |
If this case is not already tested by tests/wpt/web-platform-tests/domparsing/XMLSerializer-serializeToString.html, then a new testcase should be added. |
Hello! would like to contribute here but probably I'll need some assistance since I'm a newbie on the codebase. |
@5h1rU I think you should give it a try and ask questions about anything that is unclear :) |
@highfive assign me |
Hey @5h1rU! Thanks for your interest in working on this issue. It's now assigned to you! |
After some hours trying to figure out what's going on I have some blockers
Then I change the Document match to: NodeTypeId::Document(DocumentTypeId::Document) => {
let doc = n.downcast::<Node>().unwrap();
// What else?
}, I'm confused about the document serialization algorithm implementation. Not sure where is the best place to put the logic. I tried using the iter.push_node(doc); @jdm Do you think is possible to use one of the current existent serializer methods? In conclusion: I don't know what's the next step in order to return the document serialized. |
@5h1rU This comment means that we should not modify the match with the panic. However, we should modify the SerializationIterator |
…r=<try> Add new XMLSerializer().serializeToString functionality <!-- Please describe your changes on the following line: --> This is the fix for ScriptThread panic when `new XMLSerializer().serializeToString(document);` is called. r?@jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23130 <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23153) <!-- Reviewable:end -->
…r=jdm Make serializeToString serialize document nodes correctly <!-- Please describe your changes on the following line: --> This is the fix for ScriptThread panic when `new XMLSerializer().serializeToString(document);` is called. r?@jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23130 <!-- Either: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23153) <!-- Reviewable:end -->
…r=jdm Make serializeToString serialize document nodes correctly <!-- Please describe your changes on the following line: --> This is the fix for ScriptThread panic when `new XMLSerializer().serializeToString(document);` is called. r?@jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23130 <!-- Either: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23153) <!-- Reviewable:end -->
…r=jdm Make serializeToString serialize document nodes correctly <!-- Please describe your changes on the following line: --> This is the fix for ScriptThread panic when `new XMLSerializer().serializeToString(document);` is called. r?@jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23130 <!-- Either: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23153) <!-- Reviewable:end -->
This causes a ScriptThread panic,
Can't serialize Document node itself
servo/components/script/dom/servoparser/html.rs
Line 253 in fcd6beb
Both Chrome and Firefox properly serialize the entire page when
serializeToString
is called on the document.The text was updated successfully, but these errors were encountered: