1268047 - Change the handling of javascript: return values to align with other browsers and the updated spec

Status: RESOLVED INVALID

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

Description

[Boris Zbarsky [:bzbarsky]]

See https://github.com/whatwg/html/pull/1107

The relevant thing is that instead of doing ToString() on the return value we should just ignore it if it's not a string already.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Some javascript:/navigation web platform test fixups

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change javascript: evaluation to ignore the return value if it's not a string

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the now-unused coerce-to-string option from nsJSUtils::EvaluateOptions

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change javascript: evaluation to ignore the return value if it's not a string

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 8746155 [details] [diff] [review]
part 1.  Some javascript:/navigation web platform test fixups

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

::: testing/web-platform/tests/html/browsers/browsing-the-web/navigating-across-documents/012.html
@@ +4,5 @@
>  <script src="/resources/testharnessreport.js"></script>
>  <div id="log"></div>
>  <iframe id="test" name="test"></iframe>
> +<!-- XXX: What is this test trying to do?  It's navigating the subframe, but
> +     doing a write() to _this_ document, and the "javascript:" in there it

s/in there it/in there is/

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 8746249 [details] [diff] [review]
part 2.  Change javascript: evaluation to ignore the return value if it's not a string

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

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +279,1 @@
>          return NS_ERROR_DOM_RETVAL_UNDEFINED;

Hmm, I guess we can't just return NS_ERROR_MALFORMED_URI because nsJSChannel::AsyncOpen special-cases NS_ERROR_DOM_RETVAL_UNDEFINED? NS_ERROR_DOM_RETVAL_UNDEFINED means something returned undefined, which isn't necessarily the case here, right?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> s/in there it/in there is/

Yep, done.

> I guess we can't just return NS_ERROR_MALFORMED_URI because nsJSChannel::AsyncOpen special-cases NS_ERROR_DOM_RETVAL_UNDEFINED

Yes.

> NS_ERROR_DOM_RETVAL_UNDEFINED means something returned undefined, which isn't necessarily the case here, right?

NS_ERROR_DOM_RETVAL_UNDEFINED exists solely to make this code work.  It means whatever it needs to mean here.  I could rename it to NS_ERROR_JAVASCRIPT_URL_NOT_RESULTING_IN_DOCUMENT or something if you'd like.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a58e71d68e49
https://hg.mozilla.org/integration/mozilla-inbound/rev/54892a756c68
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a8024b450f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a58e71d68e49
https://hg.mozilla.org/mozilla-central/rev/54892a756c68
https://hg.mozilla.org/mozilla-central/rev/44a8024b450f

Comment 10

[Boris Zbarsky [:bzbarsky]]

Given that the spec this was based on is known-bogus (see <https://github.com/whatwg/html/issues/1895>) and apparently didn't follow other browsers _either_ (see <https://github.com/whatwg/html/pull/1107#issuecomment-253558923>) I'm going to back this out.  Once the spec gets sorted out, I'll think about implementing whatever it ends up saying.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Backed out on inbound.

Comment 12

[Jukka Jylänki]

Either this bug or bug 1270349 regressed being able to upload photos to dpreview.com photography community forums. See bug 1310414.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Bug 1477821 tracks updating to the spec once that's sorted out.