1362075 - Correct window.open() URL failure exception

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Anne (:annevk)]

See https://github.com/whatwg/html/issues/2490 for discussion. https://github.com/whatwg/html/issues/2626 for the specification change. And https://github.com/w3c/web-platform-tests/issues/5776 for tests (url/failure.html).

Comment 1

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Hi Alphan, would you wanna give it a try? :)

Comment 2

[Alphan Chen [:alchen]]

(In reply to Hsin-Yi Tsai (55 Regression Engineering support) [:hsinyi] from comment #1)
> Hi Alphan, would you wanna give it a try? :)
Sure !

Comment 3

[Alphan Chen [:alchen]]

I tried to run the test.

There are four kinds of failures:
1. threw object "Error: Access to 'file:///' from script denied" that is not a DOMException SyntaxError
2. assert_throws: function "() => self.open(test.input).close()" did not throw
3. threw object "[Exception... "The URI is malformed" nsresult: "0x804b000a(NS_ERROR_MALFORMED_URI)"
4. threw object "[Exception... "Unexpected error" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"

Currently, I fix some failures of "3. threw NS_ERROR_MALFORMED_URI" by modifying "netwerk/base/nsURLParsers.cpp."
Should be able to fix more in urlparser.

Comment 4

[Anne (:annevk)]

For the purposes of this bug you just need to change the exception type. The other failures are tracked by the dependencies of bug 906714, which is a much larger job (for which I'm sure your help is appreciated, if you're interested).

Comment 5

[Alphan Chen [:alchen]]

(In reply to Anne (:annevk) from comment #4)
> For the purposes of this bug you just need to change the exception type. The
> other failures are tracked by the dependencies of bug 906714, which is a
> much larger job (for which I'm sure your help is appreciated, if you're
> interested).

I am sorry that I didn't describe the status clearly.
The four kinds of failures I mentioned is only about the test(as below) you added not the whole test cases.
    self.test(() => {                                                           
      assert_throws("SyntaxError", () => self.open(test.input).close())         
    }, "window.open(): " + name)

Comment 6

[Anne (:annevk)]

Okay, but even for the tests added you can't fix all without also fixing the URL Standard issue I referenced above. In particular case 2 of comment 3 seems like something that's not really in scope for this bug.

Comment 7

[Alphan Chen [:alchen]]

(In reply to Anne (:annevk) from comment #6)
> Okay, but even for the tests added you can't fix all without also fixing the
> URL Standard issue I referenced above. In particular case 2 of comment 3
> seems like something that's not really in scope for this bug.
Yes, you are correct.

(In reply to Anne (:annevk) from comment #4)
> For the purposes of this bug you just need to change the exception type.
I could change the exception type to NS_ERROR_DOM_SYNTAX_ERR in parsing failed case.
http://searchfox.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#1152,1162,1177

By doing this, we still have some failed left.
Is this fit your expectation?

Comment 8

[Anne (:annevk)]

Yeah, the other failures are due to the URL parser not returning failure when it should.

Comment 9

[Alphan Chen [:alchen]]

Attachment: Correct window.open() URL failure exception

Based on the latest specification per comment 1, I change exception type to NS_ERROR_DOM_SYNTAX_ERR.

After applying this patch, we can pass more web platform tests.(from 88 to 105)

wpt test
http://w3c-test.org/url/failure.html

Comment 10

[Valentin Gosu [:valentin]]

Comment on attachment 8891908 [details] [diff] [review]
Correct window.open() URL failure exception

Review of attachment 8891908 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think just changing the return codes is a reliable fix for this issue. That code might change in the future.
Maybe what we should do is in nsGlobalWindow::OpenInternal - if NS_NewURI returns an error code, than return NS_ERROR_DOM_SYNTAX_ERR instead of whatever the value of rv is.

Comment 11

[Alphan Chen [:alchen]]

(In reply to Valentin Gosu [:valentin] from comment #10)
> Comment on attachment 8891908 [details] [diff] [review]
> Correct window.open() URL failure exception
> 
> Review of attachment 8891908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think just changing the return codes is a reliable fix for this
> issue. That code might change in the future.
> Maybe what we should do is in nsGlobalWindow::OpenInternal - if NS_NewURI
> returns an error code, than return NS_ERROR_DOM_SYNTAX_ERR instead of
> whatever the value of rv is.

hmm, make sense.
Thanks for your help.

Comment 12

[Alphan Chen [:alchen]]

Attachment: Correct window.open() URL failure exception

With this patch, we can fix some wpt tests of window.open.

wpt test
http://w3c-test.org/url/failure.html

Comment 13

[Alphan Chen [:alchen]]

Attachment: Correct window.open() URL failure exception. r=ehsan

Try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fbaa867a9e0274c2df75f50a67a163d343b2f72

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac10df9a0d0
Correct window.open() URL failure exception. r=ehsan

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ac10df9a0d0

Comment 16

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/4ac10df9a0d090711306661100e113f21eff268f
Bug 1362075 - Correct window.open() URL failure exception. r=ehsan