HTMLAnchorElement search setter panics due to borrow error

[jdm]

<a>anchor</a>
<script>
var a = document.querySelector('a');
a.href = "http://google.com?ho";
a.search = "?hi";
</script>

(lldb) bt
error: need to add support for DW_TAG_base_type '()' encoded with DW_ATE = 0x7, bit_size = 0
* thread #7: tid = 0x27b1b8, 0x000000010325c1f0 servo`rust_panic, stop reason = breakpoint 1.1
  * frame #0: 0x000000010325c1f0 servo`rust_panic
    frame #1: 0x0000000103251d13 servo`std::sys_common::unwind::begin_unwind_inner::h30e12d15ce2b2e25 + 467
    frame #2: 0x00000001032535bf servo`std::sys_common::unwind::begin_unwind_fmt::hb2de8a9968d38523 + 207
    frame #3: 0x00000001032636c8 servo`rust_begin_unwind + 56
    frame #4: 0x0000000103291aa1 servo`core::panicking::panic_fmt::h257ceb0aa351d801 + 129
    frame #5: 0x0000000103299566 servo`core::option::expect_failed::h2d57a5644f345e0b + 102
    frame #6: 0x000000010162bac4 servo`core::option::{{impl}}::expect<core::cell::Ref<core::option::Option<url::Url>>>(self=Option<core::cell::Ref<core::option::Option<url::Url>>> at 0x000000011abf08f8, msg=&str at 0x000000011abf08e8) + 180 at option.rs:293
    frame #7: 0x000000010162ba02 servo`script::dom::bindings::cell::{{impl}}::borrow<core::option::Option<url::Url>>(self=0x000000011ae7c310) + 66 at cell.rs:136
    frame #8: 0x000000010162bd94 servo`script::dom::htmlanchorelement::{{impl}}::update_href(self=0x000000011ae7c1c0) + 100 at htmlanchorelement.rs:86
    frame #9: 0x00000001010db8cc servo`script::dom::htmlanchorelement::{{impl}}::SetSearch(self=0x000000011ae7c1c0, value=USVString at 0x000000011abf0ac8) + 316 at htmlanchorelement.rs:411
    frame #10: 0x00000001010db732 servo`script::dom::bindings::codegen::Bindings::HTMLAnchorElementBinding::set_search(cx=0x0000000108e89900, obj=Handle<*mut js::jsapi_macos_64::JSObject> at 0x000000011abf0bb8, this=0x000000011ae7c1c0, args=JSJitSetterCallArgs at 0x000000011abf0ba8) + 338 at HTMLAnchorElementBinding.rs:886
    frame #11: 0x0000000102b6e7fb servo`CallJitSetterOp + 187
    frame #12: 0x0000000100f11cd5 servo`script::dom::bindings::utils::call_setter(info=0x0000000104ad3a40, cx=0x0000000108e89900, handle=Handle<*mut js::jsapi_macos_64::JSObject> at 0x000000011abf0cd0, this=0x000000011ae7c1c0, argc=1, vp=0x000000011abf1858) + 85 at utils.rs:524

[jdm]

Specifically, update_href calls self.url.borrow(), but it's called from inside the if let block initiated by self.url.borrow_mut() inside HTMLAnchorElement::SetSearch.

[jdm]

By inspection, this can also trigger in every setter as well. I propose that update_href accept an &Url argument and the callers all provide it, so that it doesn't need to call self.url.borrow().

Code: components/script/dom/htmlanchorelement.rs
Test: We should write one. Since it will overlap with the tests proposed in #10876, I'm fine with this living in tests/wpt/mozilla/tests/mozilla and being removed when #10876 is fixed, if this issue is fixed first.

[broesamle]

If this is not an urgent one, I would give it a try.
Since my rust practice is still young, I might take some time to find my way around...

[jdm]

@broesamle Go for it :) Ask questions if anything is confusing.

[broesamle]

Indeed, there is something that confuses me: Maybe you have something special in mind when you mention htmliframeelement.rs in particular.
The html example does not use <iframe>
and HTMLAnchorElement does not hint at any iframe-specific-ness, to me.

[jdm]

Ack, that was a typo. I meant htmlanchorelement.rs.

[broesamle]

I created a test:
$ ./mach create-wpt tests/wpt/mozilla/tests/mozilla/htmlanchorelment-search-attribute.html
The file is there but mach wouldn't run the test.

$ ./mach test-wpt --release _mozilla/mozilla/htmlanchorelement-search-attribute.html
Running tests in web-platform-tests

Unsupported test type wdspec for product servo
Ran 0 tests finished in 0.0 seconds.
  • 0 ran as expected. 0 tests skipped.

Do I have to activate it somehow?

[KiChjang]

Is the test file filled empty? It looks like you misspelt htmlanchorelement-search-attribute.html when you did ./mach create-wpt.

[broesamle]

comparing with an already existing test:
Both files are present in the same directory, one works, one does not.

$ ls tests/wpt/mozilla/tests/mozilla/html*
tests/wpt/mozilla/tests/mozilla/htmlanchorelment-search-attribute.html
[...]
tests/wpt/mozilla/tests/mozilla/htmlfontelement_size_attribute.html
[...]

$ ./mach test-wpt --release _mozilla/mozilla/htmlfontelement_size_attribute.html
Running 1 tests in web-platform-tests

Unsupported test type wdspec for product servo
Ran 1 tests finished in 5.0 seconds.
  • 1 ran as expected. 0 tests skipped.

$ ./mach test-wpt --release _mozilla/mozilla/htmlanchorelment-search-attribute.html
Running tests in web-platform-tests

Unsupported test type wdspec for product servo
Ran 0 tests finished in 0.0 seconds.
  • 0 ran as expected. 0 tests skipped.

This is the more printout of the test file:

$ more tests/wpt/mozilla/tests/mozilla/htmlanchorelment-search-attribute.html
<!doctype html>
<meta charset="utf-8">
<html>
<head>
<title>SOME TITLE TO BE ANNOUNCED</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
test(function() {
  assert_equals("X equals X", "X equals X");
});
</script>
</head>
</html>

[jdm]

Are there changes present in the MANIFEST.json that show the test file being added?

[broesamle]

git status does not tell me any changes in MANIFEST.json (which I thought was only for the tests to be expeted to fail

[jdm]

No, MANIFEST.json is where all of the tests that can be run are listed. I'm guessing you interrupted the create-wpt process before it completed? In any case, running ./mach test-wpt --manifest-update tests/wpt/mozilla/tests/mozilla/htmlanchorelment-search-attribute.html should fix it for any future runs.

[broesamle]

exactly, I have seen that before, but didn't consider it crucial for makint the test itself.
Binary path /home/helge/Development/servo/target/debug/servo does not exist

just building the debug target as well

[broesamle]

Thank you for the late night input...it worked!

[broesamle]

updated the testfile according to your suggested code
the test panics! the message looks different to your post.
how do I know its the panicking we're looking for?

Unsupported test type wdspec for product servo
  ▶ CRASH [expected OK] /_mozilla/mozilla/htmlanchorelment-search-attribute.html
  │ 
  │ thread '<main>' panicked at 'Compositor layer has an unknown pipeline (PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }).', /home/helge/Development/servo/components/compositing/compositor.rs:783
  │ stack backtrace:
  │    1:     0x55c75ca8e790 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:     0x55c75ca95dab - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:     0x55c75ca95a13 - std::panicking::default_hook::hc2c969e7453d080c
  │    4:     0x55c75c2adecf - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he75542a880009b8a
  │                         at /home/helge/Development/servo/components/util/panicking.rs:46
  │    5:     0x55c75c2add59 - _<std..thread..LocalKey<T>>::with::h9eb83d29f3539c54
  │                         at ../src/libstd/thread/local.rs:211
  │    6:     0x55c75c2adbda - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h971a6062daeeb05a
  │                         at /home/helge/Development/servo/components/util/panicking.rs:41
  │    7:     0x55c75ca7be13 - std::sys_common::unwind::begin_unwind_inner::h30e12d15ce2b2e25
  │    8:     0x55c75ca7d858 - std::sys_common::unwind::begin_unwind_fmt::hb2de8a9968d38523
  │    9:     0x55c7596df4ac - _<servo..compositing..compositor..IOCompositor<Window>>::pipeline::hbfd75b0a09bd9ecf
  │                         at /home/helge/Development/servo/components/servo/<std macros>:8
  │   10:     0x55c75978955c - _<servo..layers..layers..Layer<servo..compositing..compositor_layer..CompositorData> as servo..compositing..compositor_layer..CompositorLayer>::send_event::h01f9ab6711b91052
  │                         at /home/helge/Development/servo/components/compositing/compositor_layer.rs:404

(sorry for all the long code postings)

[jdm]

Do you see any different behaviour if you just create an HTML page anywhere and run ./mach run path/to/file.html?

[broesamle]

starting servo from the console with htmlanchorelment-search-attribute_byHand.html (which works as it should on current mozilla) i get the same as when running the corresponding test:

thread 'ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }' panicked at 'assertion failed: *self.stack == mem::transmute(&*self)', /home/helge/Development/servo/.cargo/git/checkouts/rust-mozjs-ebb4917e843c0a11/master/src/rust.rs:376
[...]

[Ms2ger]

There's another panic happening before that.

[broesamle]

Oh yes, thank you, now I see it :-)

[broesamle]

Finally, I go a working (expectedly CRASHing) minimal test.