Mozmap

[taki-jaro]

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #15012 (It remains to implement MozMapKey trait for ByteString).

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

I am stuck on how to make it possible to have ByteString as record key. To be the key in current implementation ByteString needs to be convertible from jsid and convertible to Vec<u16> (for other types it is underlying raw representation of utf-16 string). I am not sure how the conversion should be handled. Should it just convert utf-16 jsid to utf-8 and back or maybe just convert utf-16 jsid to some kind of byte by byte representation (little-endian, big-endian?). Maybe the conversion should be handled differently altogether? I would appreciate some suggestions

---

This change is 

[taki-jaro]

I resolved the issue as discussed with @jdm. Converting jsid to ByteString now just behaves like conversion from any other jsstring i.e. it fails if any characters are not in range [0, 255]. I would not modify this PR further unless there are issues raised in review

[jdm]

You should run ./mach test tests/wpt/web-platform/tests/fetch/api/headers/headers-record.html and adjust the expected test output by following https://github.com/servo/servo/blob/master/tests/wpt/README.md#updating-test-expectations.

[jdm]

Why is this operation marked unsafe?

[jdm]

I don't think the (*) is necessary around self.

[jdm]

Let's call this from_id instead.

[jdm]

Let's rename this RecordKey.

[jdm]

The implementation of this method doesn't quite match https://heycam.github.io/webidl/#es-record. Before calling JS_GetPropertyId, we need to call JS_GetOwnPropertyDescriptor and verify that the result is not undefined and enumerable. We also need to convert the key before converting the value.

[taki-jaro]

I have a problem with using JS_GetOwnPropertyDescriptorById. To call that function I should have MuableHandle and I am not sure how to obtain one in a way that will make the PropertyDescriptor rooted properly. AFAIK standard way to call such functions is doing sth like rooted!(in(cx) let mut desc = UndefinedValue()) and using desc.handle_mut() but it gives jsapi::MutableHandlejs::jsapi::Value instead of MutableHandle. I would appreciate some pointers

[jdm]

Let's rename this to Record (and this file to record.rs).

[taki-jaro]

Sorry for long delay. After review of GetOwnPropertyDescriptor from mozjs I don't think PropertyDescriptor needs rooting because there isn't created by this method. The method just uses structure given. PropertyDescriptor doesn't even have implemented traits for rooted! macro to work

As for headers-record no new tests started passing. They fail because of bad log size. I don't really understand this. Should I add some logging?

[jdm]

Could you paste the errors that you're referring to?

[jdm]

"The method just uses structure given. PropertyDescriptor doesn't even have implemented traits for rooted! macro to work"
This just means that we haven't needed them before. Unfortunately, db357d3#diff-17fcc4a0d0e726c865f88e233631d9caR123 isn't safe and we need to create a real handle from a stack root.

[taki-jaro]

As for errors I get:

  │ FAIL [expected PASS] Correct operation ordering with repeated keys
  │   → assert_equals: expected 2 but got 1
  │ 
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:262:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:247:

From assert_equals(log.length, 2); in test "Correct operation ordering with repeated keys"

[jdm]

@taki-jaro I suspect that we're not implementing the right behaviour for duplicate keys. https://bugzilla.mozilla.org/attachment.cgi?id=8837012&action=diff was a patch for this in gecko.

[jdm]

@taki-jaro More specifically, I bet our MozMap implementation does not have a defined iteration order because it relies on Rust's hashmap type. Gecko's record implementation uses an array instead, which is why the previous patch was important for it.

[taki-jaro]

@jdm: I mean most of the test fail with this or similiar error or from error from logger. I post the full tests logs below:

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Correct operation ordering with repeated keys
  │   → assert_equals: expected 2 but got 1
  │ 
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:262:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:247:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Correct operation ordering with two properties
  │   → Reflect.setPrototypeOf: expected an object or null, got symbol
  │ 
  │ loggingHandler[prop]@http://web-platform.test:8000/fetch/api/headers/headers-record.html:22:28
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:98:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:94:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Correct operation ordering with two properties one of which has an invalid value
  │   → assert_equals: expected 4 but got 1
  │ 
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:158:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:150:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Basic operation with one property
  │   → Reflect.setPrototypeOf: expected an object or null, got symbol
  │ 
  │ loggingHandler[prop]@http://web-platform.test:8000/fetch/api/headers/headers-record.html:22:28
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:47:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:43:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Correct operation ordering with undefined descriptors
  │   → Reflect.setPrototypeOf: expected an object or null, got symbol
  │ 
  │ loggingHandler[prop]@http://web-platform.test:8000/fetch/api/headers/headers-record.html:22:28
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:220:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:207:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Basic operation with Symbol keys
  │   → assert_equals: expected 7 but got 1
  │ 
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:285:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:271:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Operation with non-enumerable Symbol keys
  │   → assert_array_equals: property 1, expected "c" but got "undefined"
  │ 
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:322:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:307:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Correct operation ordering with two properties one of which has an invalid name
  │   → assert_equals: expected 5 but got 1
  │ 
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:133:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:125:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Correct operation ordering with non-enumerable properties
  │   → Reflect.setPrototypeOf: expected an object or null, got symbol
  │ 
  │ loggingHandler[prop]@http://web-platform.test:8000/fetch/api/headers/headers-record.html:22:28
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:180:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:173:1

  ▶ Unexpected subtest result in /fetch/api/headers/headers-record.html:
  │ FAIL [expected PASS] Basic operation with one property and a proto
  │   → Reflect.setPrototypeOf: expected an object or null, got symbol
  │ 
  │ loggingHandler[prop]@http://web-platform.test:8000/fetch/api/headers/headers-record.html:22:28
  │ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:73:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/fetch/api/headers/headers-record.html:68:1

[taki-jaro]

@jdm: As for rooting. I am looking into implementing it for PropertyDescriptor. But I don't exactly understand what is post_barrirer in GCMethods supposed to mean and how it is supposd to be implemented for PropertyDescriptor. Should I just call post_barrier for obj (*mut JSObject) and value (Value) as rest of the PropertyDescriptor are just uint and 2 optional functions?

[jdm]

One other way in which our implementation differs: GetPropertyKeys only passes JSITER_OWN_ONLY, but Gecko passes other flags: https://searchfox.org/mozilla-central/source/dom/bindings/Codegen.py#5107-5108

[jdm]

@taki-jaro From my reading, the post_barrier implementation for JSPropertyDescriptor can be empty.

[taki-jaro]

@jdm I am having troubles with compiling servo with rust-mozjs version 0.5.1 with my commits for PropertyDescriptor. It turns out that rust-mozjs version 0.5 made not backward-compatbile changes in recent accepted PR (changed function CollectServoSizes to take 3 arguments instead of 2) and the servo PR to update servo to work with rust-mozjs 0.5.0 looks like it has some way to be merged. The change seem not too trivial and ignoring the additional parameter and passing Nothing will change the results AFAIK.

I don't know how to proceed as my improved PR won't compile without my changes from mozjs.

[jdm]

You can use the 0.5 version by passing None to CollectServoSizes. That should be enough to allow you to continue compiling locally. Once we understand the test failures better and have addressed the implementation issues you can look at rebasing your changes.

[bors-servo]

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

[taki-jaro]

@nox, are you able to shed some light?

[jdm]

@nox Could you investigate the previous question?

[nox]

Yes, sorry that completely fell off my radar.

[nox]

@jdm I'm super lost thanks to the wrapper changes in mozjs.

[nox]

@taki-jaro From my reading, the post_barrier implementation for JSPropertyDescriptor can be empty.

Given that there is a Value in PropertyDescriptor, isn't the write barrier supposed to tell the GC about changes to that field?

[nox]

@jdm For the first question, this seems to be something deep in SpiderMonkey. I don't have anything more to say than @taki-jaro's own explanation. Do we miss a lot of Symbol related stuff in our 2 years old SM?

[jdm]

I don't know the answer to that, unfortunately.

[taki-jaro]

@jdm, @nox so what should I do with this PR? I would argue that deep changes in SpiderMonkey are out of scope though I do understand that you may be reluctant to include code with failing tests.

[nox]

I rebases it locally and am looking at the failures.

[taki-jaro]

@nox ok, thank you.

[nox]

So the issue is that for (let var in …) loops in our version of SpiderMonkey are absolutely broken, and all the closures in logHandler share the same prop value.

[nox]

Tweaking that snippet a bit, I could make the test run but still fail, because getOwnPropertyDescriptor get invoked twice in the implementation of Record.

[taki-jaro]

Ok, that's probably error on my side. Would you mind pushing these changes, so I can look into it myself?

[nox]

I don't think we can commit it as is and land such a dumbing down of the test in WPT, but you can at least apply my changes to fix the rest sure. :) Once you are done, we can temporarily clone the dumbed down version as a /_mozilla/ test and land your PR. Thanks for your continued work btw!

--- i/tests/wpt/web-platform-tests/fetch/api/headers/headers-record.html
+++ w/tests/wpt/web-platform-tests/fetch/api/headers/headers-record.html
@@ -16,7 +16,8 @@ var loggingHandler = {
 };

 setup(function() {
-  for (let prop of Object.getOwnPropertyNames(Reflect)) {
+  for (let prop_ of Object.getOwnPropertyNames(Reflect)) {
+    let prop = prop_;
     loggingHandler[prop] = function(...args) {
       addLogEntry(prop, args);
       return Reflect[prop](...args);

[taki-jaro]

@nox, @jdm, sorry for the delay, but I am completely at lost as to why it happens. It seems like JS_GetPropertyById calls JS_GetOwnPropertyDescriptorById along the way (hence the double call which disappear when I coment out if !JS_GetPropertyById(cx, object.handle(), id.handle(), property.handle_mut()) stuff). When I try to replace JS_GetPropertyById(cx, object.handle(), id.handle(), property.handle_mut()) by simply rooted!(in(cx) let mut property = desc.value); (which should have AFAIK the same results), the tests complain that there is no get called.

What is even more amusing, Gecko has it implemented in exactly the same way as in this PR i.e. first JS_GetOwnPropertyDescriptorById then JS_GetPropertyById (see https://github.com/mozilla/gecko-dev/blob/master/dom/bindings/Codegen.py#L4890) and of course the same headers-record tests (https://github.com/mozilla/gecko-dev/blob/master/testing/web-platform/tests/fetch/api/headers/headers-record.html)

[nox]

SpiderMonkey was updated since then, and I think that makes the code with let prop work as if my workaround was included.

[jdm]

Closing in favour of #24377.