Treat 'undefined' passed to optional JS arguments as missing #8854

KiChjang · 2015-12-06T07:07:31Z

@frewsxcv please don't hurt me for this.

I've added an AND condition to check whether the value being passed is undefined while checking whether the argument exists at all. Essentially, this is now treating undefined arguments the same as missing arguments.

Fixes #8813.
Fixes #6558.

KiChjang · 2015-12-06T07:07:44Z

@bors-servo try

bors-servo · 2015-12-06T07:07:45Z

⌛ Trying commit bd4515f with merge c3c2a1a...


        Auto merge of #8854 - KiChjang:undefined-as-missing, r=<try>

@frewsxcv

Treat undefined arguments in JS as missing @frewsxcv please don't hurt me for this. I've added an AND condition to check whether the value being passed is undefined while checking whehther the argument exists at all. Essentially, this is now treating undefined arguments the same as missing arguments. Fixes #8813. Fixes #6558.

KiChjang · 2015-12-06T07:18:19Z

@bors-servo try

bors-servo · 2015-12-06T07:18:21Z

⌛ Trying commit 1e84319 with merge 252e7fb...


        Auto merge of #8854 - KiChjang:undefined-as-missing, r=<try>

@frewsxcv

Treat 'undefined' passed to optional JS arguments as missing @frewsxcv please don't hurt me for this. I've added an AND condition to check whether the value being passed is undefined while checking whether the argument exists at all. Essentially, this is now treating undefined arguments the same as missing arguments. Fixes #8813. Fixes #6558.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8854)

bors-servo · 2015-12-06T08:10:10Z

💔 Test failed - linux-rel

KiChjang · 2015-12-06T08:15:21Z


Ran 3728 tests finished in 200.0 seconds.
  • 3718 ran as expected. 708 tests skipped.
  • 1 tests timed out unexpectedly
  • 9 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /dom/nodes/Comment-constructor.html:
  └ PASS [expected FAIL] new Comment(): undefined

  ▶ Unexpected subtest result in /dom/nodes/DOMImplementation-createHTMLDocument.html:
  └ PASS [expected FAIL] createHTMLDocument test 2: undefined,undefined,""

  ▶ Unexpected subtest result in /dom/nodes/Text-constructor.html:
  └ PASS [expected FAIL] new Text(): undefined

  ▶ Unexpected subtest result in /dom/traversal/TreeWalker-basic.html:
  └ PASS [expected FAIL] Construct a TreeWalker by document.createTreeWalker(root, undefined, undefined).

  ▶ Unexpected subtest result in /dom/traversal/NodeIterator.html:
  └ PASS [expected FAIL] createNodeIterator() with undefined as arguments

  ▶ Unexpected subtest result in /encoding/api-basics.html:
  └ PASS [expected FAIL] Default inputs

  ▶ Unexpected subtest result in /html/dom/documents/dom-tree-accessors/document.title-07.html:
  └ PASS [expected FAIL] createHTMLDocument test 2: undefined,undefined,""

  ▶ Unexpected subtest result in /websockets/Close-undefined.htm:
  └ PASS [expected FAIL] W3C WebSocket API - Close WebSocket - Code is undefined

  ▶ Unexpected subtest result in /websockets/Secure-Close-undefined.htm:
  └ PASS [expected FAIL] W3C WebSocket API - Close Secure WebSocket - Code is undefined

  ▶ TIMEOUT [expected PASS] /_mozilla/css/iframe/hide_layers2.html
  │ 
  │ thread 'Constellation' panicked at 'unable to find pipeline - this is a bug', ../src/libcore/option.rs:335
  │ stack backtrace:
  │    1:     0x7f5db132aa20 - sys::backtrace::tracing::imp::write::ha5f3f0d9df3eccc0xXt
  │    2:     0x7f5db132d905 - panicking::log_panic::_<closure>::closure.40991
  │    3:     0x7f5db132d380 - panicking::log_panic::hb6d5879621a98ec062x
  │    4:     0x7f5db1316683 - sys_common::unwind::begin_unwind_inner::h56feb86a7746f2474Ps
  │    5:     0x7f5db1316d38 - sys_common::unwind::begin_unwind_fmt::hbd638ea70768c304aPs
  │    6:     0x7f5db132a081 - rust_begin_unwind
  │    7:     0x7f5db135aa9f - panicking::panic_fmt::h761c3d51c4df5175ZFK
  │    8:     0x7f5dafc74a06 - constellation::_<impl>::handle_request::handle_request::h7716612666282479683
  │    9:     0x7f5dafc565c7 - sys_common::unwind::try::try_fn::try_fn::h1222482689797558573
  │   10:     0x7f5db1329ec8 - __rust_try
  │   11:     0x7f5db132629b - sys_common::unwind::try::inner_try::hb614b0fa85d9fb24CMs
  │   12:     0x7f5dafc5883a - boxed::_<impl>::call_box::call_box::h16184527091253432761
  │   13:     0x7f5db132c263 - sys::thread::_<impl>::new::thread_start::h73d591f2e83d6559I5w
  │   14:     0x7f5dad270181 - start_thread
  │   15:     0x7f5dada8a47c - __clone
  └   16:                0x0 - <unknown>

Looking good here!

KiChjang · 2015-12-06T08:15:59Z

Ran 6932 tests finished in 255.0 seconds.
  • 6930 ran as expected. 8 tests skipped.
  • 2 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /geometry-1_dev/html/DOMPoint-001.htm:
  │ FAIL [expected PASS] testConstructor2undefined
  │   → assert_equals: Expected value for y is NaN expected NaN but got 0
  │ 
  │ checkDOMPoint@http://web-platform.test:8000/geometry-1_dev/html/DOMPoint-001.htm:80:13
  │ @http://web-platform.test:8000/geometry-1_dev/html/DOMPoint-001.htm:47:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1226:20
  │ test@http://web-platform.test:8000/resources/testharness.js:427:9
  └ @http://web-platform.test:8000/geometry-1_dev/html/DOMPoint-001.htm:46:1

  ▶ Unexpected subtest result in /geometry-1_dev/html/DOMRect-001.htm:
  └ PASS [expected FAIL] testConstructorUndefined1

  ▶ Unexpected subtest result in /geometry-1_dev/html/DOMRect-001.htm:
  └ PASS [expected FAIL] testReadOnlyConstructorUndefined1

Caused a regression here but instead fixed 2 tests in its place.

KiChjang · 2015-12-06T08:28:31Z

The regression is actually REALLY weird - when a dictionary with an explicit undefined y value is used to instantiate a DOMPoint, the expected y value is 0, but when an explicit undefined is passed as a 2nd parameter to the constructor of DOMPoint, the expected y value is NaN!

Is this perhaps a spec issue?

KiChjang · 2015-12-06T08:53:51Z

Filed the regression as w3c/csswg-test#968.

frewsxcv · 2015-12-06T13:49:19Z

Not sure about the DOMPoint issue, but in general, LGTM. @nox or @Ms2ger have any thoughts here?

Ms2ger · 2015-12-06T14:31:00Z

Are we on the rust-mozjs version that checks the argument count yet? In that case we end up checking that twice here.

frewsxcv · 2015-12-06T14:40:35Z

I don't think so, I think we're on 0.1.1 and those changes ~~arrived in 0.2.0~~ haven't been pushed to crates.io yet

servo/components/servo/Cargo.lock

Line 1525 in 7d9a6a8

"js 0.1.1 (git+https://github.com/servo/rust-mozjs)",

servo/rust-mozjs#220

jdm · 2015-12-06T16:19:11Z

Actually we're using rust-mozjs as a git dependency and we're on tip of master.

frewsxcv · 2015-12-06T16:28:35Z

Yeah, you're right. Disregard my previous comment.

frewsxcv · 2015-12-06T16:30:24Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -1164,6 +1164,9 @@ def __init__(self, argument, index, args, argc, descriptorProvider,

        if not argument.variadic:
            if argument.optional:
+                # Treat undefined arguments as missing
+                undefined_condition = string.Template("!${args}.get(${index}).is_undefined()").substitute(replacer)


This should use index instead of get

Should I also change all other function calls to .get() to instead use .index()?

I'd make that a separate issue or pr

Wait a sec - Ms2ger's commit here made a better version of the .get() method. Do you still think I should use .index() instead?

Either use .get() solely here, or change your existing code to use index()

.get() returns an undefined value when it is called on a missing argument, whereas .index() will fail because of the assert!. I guess I'll stick with .get() here.

Which means I'll have to remove the previous definition of condition up there, since it's doing double-checking of parameters.

KiChjang · 2015-12-06T21:14:22Z

Using .index() didn't work for me. Compiler spitted out the following:

/home/keith/Workspace/servo/target/debug/build/script-4bfc50077d9d165b/out/Bindings/XMLHttpRequestBinding.rs:631:71: 631:79 error: no method named `index` found for type `&js::jsapi_linux_64::JSJitMethodCallArgs` in the current scope
/home/keith/Workspace/servo/target/debug/build/script-4bfc50077d9d165b/out/Bindings/XMLHttpRequestBinding.rs:631     let arg0: Option<UnionTypes::StringOrURLSearchParams > = if !args.index(0).is_undefined() {
                                                                                                                                                                                       ^~~~~~~~                                                                                        
/home/keith/Workspace/servo/components/script/dom/bindings/mod.rs:158:9: 158:64 note: in this expansion of include!

KiChjang · 2015-12-06T21:15:56Z

I've also updated the PR to remove some failing expectations and removed the line where we were doing double-checking against argument counts. Note that currently it won't land because of the CSSWG test issue.


        Auto merge of #8854 - KiChjang:undefined-as-missing, r=<try>

@frewsxcv

Treat 'undefined' passed to optional JS arguments as missing @frewsxcv please don't hurt me for this. I've added an AND condition to check whether the value being passed is undefined while checking whether the argument exists at all. Essentially, this is now treating undefined arguments the same as missing arguments. Fixes #8813. Fixes #6558.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8854)

KiChjang · 2015-12-11T21:30:22Z

@bors-servo try

bors-servo · 2015-12-11T21:30:24Z

⌛ Trying commit 41000e3 with merge 304fda1...


        Auto merge of #8854 - KiChjang:undefined-as-missing, r=<try>

@frewsxcv

Treat 'undefined' passed to optional JS arguments as missing @frewsxcv please don't hurt me for this. I've added an AND condition to check whether the value being passed is undefined while checking whether the argument exists at all. Essentially, this is now treating undefined arguments the same as missing arguments. Fixes #8813. Fixes #6558.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8854)

bors-servo · 2015-12-11T22:38:13Z

💔 Test failed - mac-rel-css

KiChjang · 2015-12-11T22:42:09Z

Ran 7097 tests finished in 878.0 seconds.
  • 7096 ran as expected. 8 tests skipped.
  • 1 tests crashed unexpectedly

Tests with unexpected results:
  ▶ CRASH [expected PASS] /css21_dev/html4/margin-bottom-applies-to-002.htm
  │ 
  │ was ready to save, resetting ready_to_save_state
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ thread 'ScriptTask PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Custom(Custom { kind: Other, error: StringError("recursive IPC channel use during serialization") }) }', ../src/libcore/result.rs:741
  │ stack backtrace:
  │    1:        0x10358dbf8 - sys::backtrace::tracing::imp::write::hca66179add43b2e5lLt
  │    2:        0x10358fcbf - panicking::log_panic::_<closure>::closure.40842
  │    3:        0x10358f762 - panicking::log_panic::h7a04edac7e1f2797HEx
  │    4:        0x10357b606 - sys_common::unwind::begin_unwind_inner::h39a94087039ae0efnOs
  │    5:        0x10357b9ee - sys_common::unwind::begin_unwind_fmt::h7be0824d83765cd5tNs
  │    6:        0x10358d217 - rust_begin_unwind
  │    7:        0x1035b22a0 - panicking::panic_fmt::h871c0ef100fda9e9LFK
  │    8:        0x102c39587 - mem::_<impl>::send::haf009788f0ee7115Bea
  │    9:        0x10279e430 - sys_common::unwind::try::try_fn::try_fn::h6135003471651053147
  │   10:        0x10358d038 - __rust_try
  │   11:        0x10358a13e - sys_common::unwind::try::inner_try::h819d2da8e3dc4516VKs
  │   12:        0x10279f601 - boxed::_<impl>::call_box::call_box::h3509381201889269884
  │   13:        0x10358ef4d - sys::thread::_<impl>::new::thread_start::h328957c252b95d3cOYw
  │   14:     0x7fff8416a059 - _pthread_body
  │   15:     0x7fff84169fd6 - _pthread_start
  │ thread 'ScriptTask PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }' panicked at 'called `Option::unwrap()` on a `None` value', ../src/libcore/option.rs:367
  │ stack backtrace:
  │    1:        0x10358dbf8 - sys::backtrace::tracing::imp::write::hca66179add43b2e5lLt
  │    2:        0x10358fcbf - panicking::log_panic::_<closure>::closure.40842
  │    3:        0x10358f762 - panicking::log_panic::h7a04edac7e1f2797HEx
  │    4:        0x10357b504 - sys_common::unwind::begin_unwind_inner::h39a94087039ae0efnOs
  │    5:        0x10357b9ee - sys_common::unwind::begin_unwind_fmt::h7be0824d83765cd5tNs
  │    6:        0x10358d217 - rust_begin_unwind
  │    7:        0x1035b22a0 - panicking::panic_fmt::h871c0ef100fda9e9LFK
  │    8:        0x1035b259c - panicking::panic::hbe2e3a5159c3d178iEK
  │    9:        0x102354039 - page::_<impl>::window::h813c8df4d8dc356dTuZ
  │   10:        0x10279b504 - script_task::_<impl>::drop::h437e092b646f5d2cLH0
  │   11:        0x10279ef50 - sys_common::unwind::try::try_fn::try_fn::h6135003471651053147
  │   12:        0x10358d038 - __rust_try
  │   13:        0x10358a13e - sys_common::unwind::try::inner_try::h819d2da8e3dc4516VKs
  │   14:        0x10279f601 - boxed::_<impl>::call_box::call_box::h3509381201889269884
  │   15:        0x10358ef4d - sys::thread::_<impl>::new::thread_start::h328957c252b95d3cOYw
  │   16:     0x7fff8416a059 - _pthread_body
  │   17:     0x7fff84169fd6 - _pthread_start
  └ thread panicked while panicking. aborting.

What is this?

Ms2ger · 2015-12-11T23:24:23Z

@bors-servo retry

#8945

bors-servo · 2015-12-11T23:24:25Z

⌛ Trying commit 41000e3 with merge 2660b9f...


        Auto merge of #8854 - KiChjang:undefined-as-missing, r=<try>

@frewsxcv

Treat 'undefined' passed to optional JS arguments as missing @frewsxcv please don't hurt me for this. I've added an AND condition to check whether the value being passed is undefined while checking whether the argument exists at all. Essentially, this is now treating undefined arguments the same as missing arguments. Fixes #8813. Fixes #6558.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8854)

bors-servo · 2015-12-12T00:15:19Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

frewsxcv · 2015-12-12T15:21:10Z

components/script/dom/bindings/codegen/CodegenRust.py, line 1168 [r1] (raw file):
I usually simplify conditionals in the form of if !a { b } else { c } to the form of if a { c } else { b }, but it's not a big deal

Another consideration here would be to link to the spec (step 10.4)

Comments from the review on Reviewable.io

frewsxcv · 2015-12-12T15:21:31Z

Looks good to me. Anyone else have thoughts on this?

KiChjang · 2015-12-13T02:21:49Z

Review status: 0 of 15 files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/bindings/codegen/CodegenRust.py, line 1168 [r1] (raw file):
You're totally right. It's pretty easy to miss the exclamation mark when reading this.

Comments from the review on Reviewable.io

frewsxcv · 2015-12-13T05:04:56Z

@bors-servo delegate=KiChjang

r=me once you fix that conditional

bors-servo · 2015-12-13T05:04:58Z

✌️ @KiChjang can now approve this pull request

KiChjang · 2015-12-13T05:11:54Z

@bors-servo r=frewsxcv

bors-servo · 2015-12-13T05:11:55Z

📌 Commit db92a8b has been approved by frewsxcv

bors-servo · 2015-12-13T05:11:57Z

⌛ Testing commit db92a8b with merge 8bab1cd...


        Auto merge of #8854 - KiChjang:undefined-as-missing, r=frewsxcv

@frewsxcv

Treat 'undefined' passed to optional JS arguments as missing @frewsxcv please don't hurt me for this. I've added an AND condition to check whether the value being passed is undefined while checking whether the argument exists at all. Essentially, this is now treating undefined arguments the same as missing arguments. Fixes #8813. Fixes #6558.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8854)


        Treat undefined arguments in JS as missing

bors-servo · 2015-12-13T06:02:48Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Dec 6, 2015

KiChjang mentioned this pull request Dec 6, 2015

Treat 'undefined' passed to an optional argument as "missing" #8849

Closed

KiChjang changed the title ~~Treat undefined arguments in JS as missing~~ Treat undefined passed in optional JS arguments as missing Dec 6, 2015

KiChjang changed the title ~~Treat undefined passed in optional JS arguments as missing~~ Treat 'undefined' passed to optional JS arguments as missing Dec 6, 2015

KiChjang force-pushed the KiChjang:undefined-as-missing branch from bd4515f to 1e84319 Dec 6, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 6, 2015

KiChjang force-pushed the KiChjang:undefined-as-missing branch from 1e84319 to 4fabc09 Dec 6, 2015

highfive removed the S-tests-failed label Dec 6, 2015

frewsxcv reviewed Dec 6, 2015
View changes

KiChjang force-pushed the KiChjang:undefined-as-missing branch from 4fabc09 to e29fcda Dec 6, 2015

KiChjang mentioned this pull request Dec 6, 2015

Replicate CallArgs::{get, index, index_mut} onto JSJitMethodCallArgs servo/rust-mozjs#223

Merged

KiChjang force-pushed the KiChjang:undefined-as-missing branch from b1fd0ef to 41000e3 Dec 11, 2015

highfive removed the S-tests-failed label Dec 11, 2015

highfive added the S-tests-failed label Dec 11, 2015

jdm removed the S-tests-failed label Dec 12, 2015

KiChjang force-pushed the KiChjang:undefined-as-missing branch from 41000e3 to db92a8b Dec 13, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 13, 2015

Treat undefined arguments in JS as missing

Loading status checks…

db92a8b

KiChjang deleted the KiChjang:undefined-as-missing branch Dec 13, 2015

servo / servo

Join GitHub today

Treat 'undefined' passed to optional JS arguments as missing #8854

Treat 'undefined' passed to optional JS arguments as missing #8854

Conversation

KiChjang commented Dec 6, 2015

KiChjang commented Dec 6, 2015

bors-servo commented Dec 6, 2015

KiChjang commented Dec 6, 2015

bors-servo commented Dec 6, 2015

bors-servo commented Dec 6, 2015

KiChjang commented Dec 6, 2015

KiChjang commented Dec 6, 2015

KiChjang commented Dec 6, 2015

KiChjang commented Dec 6, 2015

frewsxcv commented Dec 6, 2015

Ms2ger commented Dec 6, 2015

frewsxcv commented Dec 6, 2015

jdm commented Dec 6, 2015

frewsxcv commented Dec 6, 2015

This comment has been minimized.

frewsxcv Dec 6, 2015

This comment has been minimized.

KiChjang Dec 6, 2015

This comment has been minimized.

frewsxcv Dec 6, 2015

This comment has been minimized.

KiChjang Dec 6, 2015

This comment has been minimized.

frewsxcv Dec 6, 2015

This comment has been minimized.

KiChjang Dec 6, 2015

KiChjang commented Dec 6, 2015

KiChjang commented Dec 6, 2015

KiChjang commented Dec 11, 2015

bors-servo commented Dec 11, 2015

bors-servo commented Dec 11, 2015

KiChjang commented Dec 11, 2015

Ms2ger commented Dec 11, 2015

bors-servo commented Dec 11, 2015

bors-servo commented Dec 12, 2015

frewsxcv commented Dec 12, 2015

frewsxcv commented Dec 12, 2015

KiChjang commented Dec 13, 2015

frewsxcv commented Dec 13, 2015

bors-servo commented Dec 13, 2015

KiChjang commented Dec 13, 2015

bors-servo commented Dec 13, 2015

bors-servo commented Dec 13, 2015

bors-servo commented Dec 13, 2015