Closed Bug 788369 Opened 8 years ago Closed 8 years ago

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

Categories

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

16 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 + verified
firefox17 + verified
firefox18 + verified

People

(Reporter: lorchard, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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
Blocks: 692677
Keywords: regression
Version: 18 Branch → 16 Branch
Component: Untriaged → Security
Product: Firefox → Core
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.
Component: Security → DOM
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.
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?
(Btw, thanks for fixing, bz!)
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
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.
(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.
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 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.
Attachment #658359 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f60894b8355
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla18
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
Attachment #658359 - Flags: approval-mozilla-beta?
Attachment #658359 - Flags: approval-mozilla-aurora?
Attachment #659278 - Flags: approval-mozilla-beta?
Attachment #659278 - Flags: approval-mozilla-aurora?
Attachment #658359 - Attachment is obsolete: true
Attachment #658359 - Flags: approval-mozilla-beta?
Attachment #658359 - Flags: approval-mozilla-aurora?
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.
Attachment #659278 - Flags: approval-mozilla-beta?
Attachment #659278 - Flags: approval-mozilla-beta+
Attachment #659278 - Flags: approval-mozilla-aurora?
Attachment #659278 - Flags: approval-mozilla-aurora+
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!
Keywords: verifyme
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.
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0b1
QA Contact: paul.silaghi
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0b1
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.