788369 - In Dropbox JS sample page, NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument

Status: VERIFIED FIXED

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

Description

[Les Orchard [:lorchard]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20120904030512

Steps to reproduce:

I attempted to load this web page, as part of the new Dropbox JS client code:

https://dl-web.dropbox.com/spa/pjlfdak1tmznswp/powered_by.js/public/index.html


Actual results:

In Nightly 18.0a1 (2012-09-04) and Aurora 17.0a2 (2012-09-04) on Win7, the page came up blank, with this error in the Web Console:

[21:11:16.264] NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument @ https://dl-web.dropbox.com/spa/pjlfdak1tmznswp/powered_by.js/public/lib/coffee-script.js:8

Unfortunately, that coffee-script.js is minified, so this error is probably not much help. I've also no idea what this error means. :/


Expected results:

In Release 15.0 on Win 7, the page loaded as expected.

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/5891cc95eac7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120607140205
Bad:
http://hg.mozilla.org/mozilla-central/rev/22bb7d46bb23
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120608081921
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5891cc95eac7&tochange=22bb7d46bb23


Regresiion window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3638e7addad9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120607112919
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/57c5763ac3ac
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120607113019
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3638e7addad9&tochange=57c5763ac3ac

Triggered by:
57c5763ac3ac	Philipp von Weitershausen — Bug 692677 - Relax same-origin XHR restrictions for privileged applications. r=sicking

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, this is fun.  The minified coffeescript thing has this:

  c = new(window.ActiveXObject || XMLHttpRequest)("Microsoft.XMLHTTP")

So it's basically relying on the fact that passing the string to the XHR constructor used to be a no-op.  But now we expect a dictionary there, and in particular we end up throwing due to the arg not being null, undefined, or object in http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary step 1.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow passing strings to the XHR constructor, since CoffeeScript seems to want to do it.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For the love of...

Comment 5

[Boris Zbarsky [:bzbarsky]]

And just so you know, this is part of the CoffeScript "standard library", not a minifier artifact.  See http://coffeescript.org/documentation/docs/browser.html the source for CoffeeScript.load.

Comment 6

[Philipp von Weitershausen [:philikon]]

Let's see if they take my fix: https://github.com/jashkenas/coffee-script/issues/2534

Makes me wonder how many other bad constructor arguments are out there, though. I definitely vote for taking bz's fix. Maybe we can report a warning to the Web Developer Console if a string argument is passed?

Comment 7

[Philipp von Weitershausen [:philikon]]

(Btw, thanks for fixing, bz!)

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 658359 [details] [diff] [review]
Allow passing strings to the XHR constructor, since CoffeeScript seems to want to do it.

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

::: dom/webidl/XMLHttpRequest.webidl
@@ +51,5 @@
>    boolean mozSystem = false;
>  };
>  
> +[Constructor(optional MozXMLHttpRequestParameters params),
> + // There are apparently callers, specifically CoffeScript, who do

Coffee

Comment 9

[Boris Zbarsky [:bzbarsky]]

Philipp, thanks for filing the upstream bug!

Warning in the dev console is doable, but would take some extra code.  Not sure if it's necessarily worth it.  Maybe if it makes people update their coffeescript bits...

Ms2ger: will fix.

Comment 10

[Les Orchard [:lorchard]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Oh, this is fun.  The minified coffeescript thing has this:
> 
>   c = new(window.ActiveXObject || XMLHttpRequest)("Microsoft.XMLHTTP")
> 
> So it's basically relying on the fact that passing the string to the XHR
> constructor used to be a no-op.  But now we expect a dictionary there, and
> in particular we end up throwing due to the arg not being null, undefined,
> or object in http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary step 1.

Thank you very much for filling in the blanks for me! I was hacking on something else entirely when I ran into this. I saw the minified coffee-script.js, then just kind of flailed & left for ice cream rather than hunt for the bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Makes sense.  ;)

For what it's worth, http://jsbeautifier.org/ did a pretty good job with this one.  We got lucky because the script is just sort of whitespace-challenged, not seriously minified.

And thank you very much for filing this, by the way.  _Really_ glad we caught this now, not after we shipped the bug!

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 658359 [details] [diff] [review]
Allow passing strings to the XHR constructor, since CoffeeScript seems to want to do it.

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

Unfortunate that we have to do this, but oh well.

::: dom/bindings/test/test_bug788369.html
@@ +12,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=788369">Mozilla Bug 788369</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

Trailing whitespace.

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f60894b8355

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 658359 [details] [diff] [review]
Allow passing strings to the XHR constructor, since CoffeeScript seems to want to do it.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 692677
User impact if declined: Sites using CoffeeScript are likely to break.
Testing completed (on m-c, etc.): Passes attached test, fixes sites in question.
Risk to taking this patch (and alternatives if risky): This should be low-risk. 
  It just adds another way to call the XMLHttpRequest constructor; a way that
  used to work before bug 692677.
String or UUID changes made by this patch: None

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow passing strings to the XHR constructor, since CoffeeScript seems to want to do it.

Comment 16

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/de4e95e2ccc3 to fix test bustage.  That's rolled into the patch I asked for approvals on.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f60894b8355
https://hg.mozilla.org/mozilla-central/rev/de4e95e2ccc3

Comment 18

[Les Orchard [:lorchard]]

FWIW: The original issue I saw in the dropbox-js code that prompted me to file this bug appears to be resolved in Nightly, right now. Thank you!

Comment 19

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41696b70b514
https://hg.mozilla.org/releases/mozilla-beta/rev/237128db4339

Comment 20

[Paul Silaghi, QA [:pauly]]

Saw the issue on Nightly 18.0a1 (2012-09-04). Page loaded, no errors in the web console on FF 16b4 on Win 7 x64. Verified fixed.

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0b1

Comment 22

[Paul Silaghi, QA [:pauly]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0b1