Closed
Bug 788369
Opened 12 years ago
Closed 12 years ago
In Dropbox JS sample page, NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla18
People
(Reporter: lorchard, Assigned: bzbarsky)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → ?
tracking-firefox18:
--- → ?
Keywords: regression
Version: 18 Branch → 16 Branch
Updated•12 years ago
|
Component: Untriaged → Security
Product: Firefox → Core
Assignee | ||
Comment 2•12 years ago
|
||
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 | ||
Comment 3•12 years ago
|
||
Attachment #658359 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bzbarsky
Whiteboard: [need review]
For the love of...
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(Btw, thanks for fixing, bz!)
Comment 8•12 years ago
|
||
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
Assignee | ||
Comment 9•12 years ago
|
||
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.
Reporter | ||
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Comment 11•12 years ago
|
||
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!
Updated•12 years ago
|
Comment 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•12 years ago
|
||
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla18
Assignee | ||
Comment 14•12 years ago
|
||
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?
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #659278 -
Flags: approval-mozilla-beta?
Attachment #659278 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #658359 -
Attachment is obsolete: true
Attachment #658359 -
Flags: approval-mozilla-beta?
Attachment #658359 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 16•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2f60894b8355
https://hg.mozilla.org/mozilla-central/rev/de4e95e2ccc3
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #659278 -
Flags: approval-mozilla-beta?
Attachment #659278 -
Flags: approval-mozilla-beta+
Attachment #659278 -
Flags: approval-mozilla-aurora?
Attachment #659278 -
Flags: approval-mozilla-aurora+
Reporter | ||
Comment 18•12 years ago
|
||
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!
Assignee | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/41696b70b514
https://hg.mozilla.org/releases/mozilla-beta/rev/237128db4339
status-firefox16:
--- → fixed
status-firefox17:
--- → fixed
Updated•12 years ago
|
status-firefox18:
--- → fixed
Comment 20•12 years ago
|
||
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•12 years ago
|
||
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0b1
QA Contact: paul.silaghi
Comment 22•12 years ago
|
||
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0b1
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•