Ak 18468

[toidiu]

WIP

We panic in function string_jsid_to_string if HandleId is not a string. This PR attempts to fix this by testing if the id is not a string prior to calling the function.

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #18468 (github issue number if applicable).

• There are tests for these changes OR
• These changes do not require tests because _____

---

This change is 

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/conversions.rs
• @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/conversions.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[toidiu]

@jdm three things:

• I couldn't use match because then I was having problems with scope created by the match block.
• Currently this doesnt actually fix the issue!! I have a few println statements that show the data that is causing the error(posted below).
• I think a better way to check is to simply call if !RUST_JSID_IS_STRING(id) since thats where the assertion fails. I am going to make a PR to show this and wondering if there is anything wrong with this approach.

jsid_to_string(cx, id).is_none() is true for all of these cases.

**GOOD SCENARIO**
Some(DOMString("hasAttribute", PhantomData))
Some(DOMString("jQuery2130307340275337905872", PhantomData))
**BAD SCENARIO**
Some(DOMString("0", PhantomData))

[toidiu]

commit 7ccd2c7 uses RUST_JSID_IS_STRING for the if condition and fixes the panic.

We still cant successfully load http://101xp.com, servo just seems to hang. However, I dont think that is within the scope of this issue so I am going to proceed with this fix and write tests for this.

[KiChjang]

I believe #18543 solves the same issue as well.

[bors-servo]

☔ The latest upstream changes (presumably #18543) made this pull request unmergeable. Please resolve the merge conflicts.

[toidiu]

@KiChjang so currently the in the upstream change we will panic with an except. Do we instead want to return false? Let me know if this issue is still valid or if it can be closed.

[emilio]

@toidiu we can't return false because it'll stop the execution of the script. Instead we need to figure out from where is it arriving with a non-string JSID, and see what the correct behavior should be.

We can do it on this bug, let me know if you need any help, happy to try helping :)

[emilio]

In particular, this comes from CGDOMJSProxyHandler_delete.

This should implement the following: https://heycam.github.io/webidl/#legacy-platform-object-delete

So there seems to be some stuff missing. In particular:

• Indexed properties check (you can skip this from this PR and just leave a FIXME).
• A STRING / INT check before calling CGProxyNamedDeleter(self.descriptor).define(). This is what causes the crash.

[emilio]

Also, this of course needs a test-case.

[emilio]

So, here's a test-case, and now I'm no longer sure adding that is the correct fix.

In particular, looks to me that we should just be handling atoms JSIDs...

<script>
  delete window.localStorage[Symbol()];
</script>

[toidiu]

Sry my productivity drops during the week because of my job but I am keeping an eye on this.

[KiChjang]

@toidiu Merge commits are not accepted. Please rebase against master instead.

[toidiu]

@emilio I can't get the latest servo to crash anymore..

RUST_BACKTRACE=1; ./mach run http://101xp.com doesnt crash. Dont know what I am fixing at this point.

Can you point me to where I would add the test to cause the crash.

[emilio]

Doesn't running the test-case above (a file test.html, with the content I pasted) cause the crash?

[KiChjang]

@toidiu Have you tried solving the test case that @emilio has commented?

[wafflespeanut]

Closing due to inactivity.