Closed Bug 918707 Opened 11 years ago Closed 8 years ago

[XHR2] Wrong exception: "The URI is malformed" instead of "DOMException SyntaxError"

Categories

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

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: hsteen, Assigned: shawnjohnjr)

References

()

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file, 2 obsolete files)

Test case that fails because the wrong type of exception is thrown:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/open-url-bogus.htm
(In reply to Hallvord R. M. Steen [:hallvors] from comment #0)
> Test case that fails because the wrong type of exception is thrown:
> http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/open-url-bogus.
> htm

http://w3c-test.org/XMLHttpRequest/open-url-bogus.htm
Assignee: nobody → shuang
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Throws a SyntaxError exception if either method is not a valid HTTP method or url cannot be parsed.
I can pass the second test case, via returning value NS_ERROR_DOM_SYNTAX_ERR when rv is NS_ERROR_MALFORMED_URI, but the first case failed.
The first test case pass url 'http:', and nsIOService::NewURI returns NS_OK. I'm checking nsHttpHandler now.
The first test case failed because we treat "http:" as the deprecated form of relative urls.

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#2146
Very strange, the code fall through here [1]. It's suppose to be '\0'.
Checking |*realrelpath| in gdb, it's empty string.

[1]https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#2182
(In reply to Shawn Huang [:shawnjohnjr] from comment #5)
> Very strange, the code fall through here [1]. It's suppose to be '\0'.
> Checking |*realrelpath| in gdb, it's empty string.
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.
> cpp#2182

Maybe something wrong with my gdb. Now it falls through '\0'.
I'm a bit not sure why 'relative' url is bogus. I did not see the specification says XHR request url can not be 'relative' url.
Honestly I don't understand why [1] commented 'deprecated form of relative urls'. 

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#2143

In the document, RFC 1808, relativeURL doesn't contain scheme.
 
   URL         = ( absoluteURL | relativeURL ) [ "#" fragment ]
   absoluteURL = generic-RL | ( scheme ":" *( uchar | reserved ) )
   generic-RL  = scheme ":" relativeURL
   relativeURL = net_path | abs_path | rel_path

So I don't see any reason why we treat "http:" as relative URL.
Is there any historical reason here?
Flags: needinfo?(khuey)
I don't know, this code is really old.

From our perspective as the DOM team, the actual URLs are less important than throwing the wrong exception.  I would just fix us to throw the right exception when we do throw, and not worry so much about whether or not we accept some URLs we shouldn't, even if that means the test still doesn't fully pass.
Flags: needinfo?(khuey)
As Comment 9 suggested, this patch can only fix the wrong exception, and the url "http:" is still acceptable.
Attachment #8736285 - Flags: review?(khuey)
Comment on attachment 8736285 [details] [diff] [review]
Bug 918707 - Return syntax error if url is invalid

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

r=me

Should there be changes to test manifests too?
Attachment #8736285 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> Comment on attachment 8736285 [details] [diff] [review]
> Bug 918707 - Return syntax error if url is invalid
> 
> Review of attachment 8736285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> Should there be changes to test manifests too?
You're right. I will change open-url-bogus.htm.ini.
I'm not sure this is something related to my patch.


800 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_bug484396.html | xhr.open() succeeds with empty url
801 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_bug484396.html | uncaught exception - InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable at @http://mochi.test:8888/tests/dom/base/test/test_bug484396.html:44:1
1050 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_bug814576.html | uncaught exception - SyntaxError: An invalid or illegal string was specified at @http://mochi.test:8888/tests/dom/base/test/test_bug814576.html:23:1
1395 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_xhr_send_readystate.html | uncaught exception - SyntaxError: An invalid or illegal string was specified at @http://mochi.test:8888/tests/dom/base/test/test_xhr_send_readystate.html:29:1
So I misunderstood the XHR specification, empty string is accept and it shall be replaced as document url based on the discussion in bug 484396.
https://hg.mozilla.org/mozilla-central/rev/4c17ed03e401
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.