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)
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
Whiteboard: [tw-dom]
Assignee | ||
Comment 1•8 years ago
|
||
(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 | ||
Updated•8 years ago
|
Assignee: nobody → shuang
Updated•8 years ago
|
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Assignee | ||
Comment 2•8 years ago
|
||
Throws a SyntaxError exception if either method is not a valid HTTP method or url cannot be parsed.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
(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'.
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Attachment #8736285 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22b8fab16de0
Assignee | ||
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8737069 -
Attachment is obsolete: true
Assignee | ||
Comment 17•8 years ago
|
||
Don't return syntax error if url is empty.
Assignee | ||
Comment 18•8 years ago
|
||
Push to try-server again. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b4a4cf17674
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c17ed03e401
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•