Last Comment Bug 613634 - NS_ERROR_XPC_NOT_ENOUGH_ARGS on rumpetroll.com (investigate if we should have optional useCapture parameter)
: NS_ERROR_XPC_NOT_ENOUGH_ARGS on rumpetroll.com (investigate if we should have...
Status: RESOLVED FIXED
[parity-webkit][parity-opera][parity-...
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Olli Pettay [:smaug]
:
Mentors:
: 646687 (view as bug list)
Depends on: 654770
Blocks: 616252
  Show dependency treegraph
 
Reported: 2010-11-19 15:20 PST by Josh Matthews [:jdm]
Modified: 2013-11-12 00:57 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
optional capture param (4.75 KB, patch)
2010-11-21 10:58 PST, Olli Pettay [:smaug]
no flags Details | Diff | Review
patch (18.67 KB, patch)
2011-04-23 06:15 PDT, Olli Pettay [:smaug]
jonas: review+
Details | Diff | Review

Description Josh Matthews [:jdm] 2010-11-19 15:20:06 PST
STR:
1. Visit rumpetroll.com

Expected:
Swimming around in a fun websocket-based game

Actual:
The error appears in the console, no game is present.

This works fine in chrome.
Comment 1 Josh Matthews [:jdm] 2010-11-19 15:21:31 PST
This is the failing line in main.js:

>document.getElementById('authorize-user-button').addEventListener('click', function(e) { 
>		app.authorize(null,null);
>		authWindow = window.open("auth.html","","width=950,height=460,menubar=no,toolbar=no,location=no,directories=no,status=no,scrollbars=yes,resizable=yes')")
>		return false;
>	});
Comment 2 Josh Matthews [:jdm] 2010-11-19 15:32:46 PST
This error is occurring because |addEventListener('click', function(e) {})| throws.  CCing people who might know whether this is correct behaviour or not.
Comment 3 Boris Zbarsky [:bz] 2010-11-19 19:02:41 PST
It's correct behavior per DOM 2 Events, and this site is assuming Webkit's behavior which doesn't follow that spec, really.

There have been proposals to change it for Web DOM Core; I don't believe we have a bug on making the third arg optional so far.
Comment 5 Olli Pettay [:smaug] 2010-11-21 10:58:08 PST
Created attachment 492203 [details] [diff] [review]
optional capture param

I pushed this to tryserver
Comment 6 Olli Pettay [:smaug] 2010-11-21 13:06:09 PST
Apparently that kind of simple patch breaks quite a few tests.
Comment 7 Boris Zbarsky [:bz] 2010-11-21 13:08:24 PST
Well, at least the ones in the official DOM test suites, right?
Comment 8 :Ms2ger 2010-11-21 13:32:50 PST
http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorkerMessageHandler.cpp#339

342 NS_IMETHODIMP
343 nsDOMWorkerMessageHandler::AddEventListener(const nsAString& aType,
344                                             nsIDOMEventListener* aListener,
345                                             PRBool aUseCapture,
346                                             PRBool aWantsUntrusted,
347                                             PRUint8 optional_argc)
348 {
349   // We don't support aWantsUntrusted yet.
350   NS_ENSURE_TRUE(optional_argc == 0, NS_ERROR_NOT_IMPLEMENTED);

This isn't going to fly anymore...
Comment 9 Olli Pettay [:smaug] 2010-12-04 03:42:37 PST
We may want to fix this for FF4.
If I understood Travis (from MS) correctly, IE9 may make the useCapture
optional too.
Comment 10 Cameron McCormack (:heycam) 2010-12-07 12:43:07 PST
As mentioned in http://www.w3.org/mid/20101124025438.GA14523@wok.mcc.id.au we came to the conclusion that Web IDL should allow too few arguments, and coerce them from undefined, (and allow too many, and ignore the extra arguments) as the most JavaScript-y way to handle mismatched arity.  Nobody has spoken up against this proposal on the list yet.  (The change is yet to have been made to the spec.)

The result of this is that it wouldn't matter if useCapture is optional or not -- omitting it would require the missing argument to be treated as undefined and thus be converted to false.
Comment 11 Olli Pettay [:smaug] 2010-12-13 14:21:25 PST
Not sure if this really needs to *block*.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2010-12-13 14:59:41 PST
Agreed, we can get to this later.
Comment 13 Frank Yan (:fryn) 2011-04-13 13:30:12 PDT
Is there a technical or spec holdup to fixing this?

We've already made the second parameter of window.getComputedStyle to be optional.
Comment 14 Olli Pettay [:smaug] 2011-04-13 13:56:54 PDT
(In reply to comment #13)
> Is there a technical or spec holdup to fixing this?
No. I just need to find time to fix this.
Comment 15 Cameron McCormack (:heycam) 2011-04-13 13:58:41 PDT
Also note that Web DOM Core and DOM 3 Events have changed to make the argument optional:

  http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#eventtarget
  http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-EventTarget-addEventListener
Comment 16 Boris Zbarsky [:bz] 2011-04-13 17:50:50 PDT
Olli, is there anything to do here other than adding "optional" to the idl and writing tests?
Comment 17 Olli Pettay [:smaug] 2011-04-14 00:56:08 PDT
I did try that [optional] approach and something broke.
The reason is, IIRC, that we already have one optional parameter, so need to
update optional_argc handling.
Comment 18 Frank Yan (:fryn) 2011-04-14 01:03:08 PDT
(In reply to comment #17)
> I did try that [optional] approach and something broke.
> The reason is, IIRC, that we already have one optional parameter, so need to
> update optional_argc handling.

This problem only affects the nsIDOMNSEventTarget interface, which web content doesn't use, AIUI. Could we could first land a patch that makes useCapture optional only for nsIDOMEventTarget?
Comment 19 Olli Pettay [:smaug] 2011-04-14 01:14:12 PDT
Web content does use addEventListener from nsIDOMNSEventTarget.
addEventListener in nsIDOMEventTarget is [noscript]
Comment 20 Olli Pettay [:smaug] 2011-04-23 06:15:21 PDT
Created attachment 527936 [details] [diff] [review]
patch

Uploaded to tryserver
Comment 21 :Ms2ger 2011-05-02 05:25:37 PDT
*** Bug 646687 has been marked as a duplicate of this bug. ***
Comment 22 Jonas Sicking (:sicking) 2011-05-03 12:07:18 PDT
Comment on attachment 527936 [details] [diff] [review]
patch

Maybe also test that

x.addEventListener("foo", l);
x.removeEventListener("foo, l, false);

removes the event listener?
Comment 23 Olli Pettay [:smaug] 2011-05-07 10:58:54 PDT
http://hg.mozilla.org/mozilla-central/rev/f0ad024522bb
Comment 24 Eric Shepherd [:sheppy] 2011-05-10 06:32:25 PDT
Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMEventTarget
https://developer.mozilla.org/en/DOM/element.addEventListener

And mentioned on Firefox 6 for developers.

Note You need to log in before you can comment on or make changes to this bug.