removeEventListener broken
--------------------------
Key: CB-82
URL: https://issues.apache.org/jira/browse/CB-82
Project: Apache Callback
Issue Type: Bug
Components: weinre
Reporter: Patrick Mueller
Assignee: Patrick Mueller
from: https://github.com/callback/callback-weinre/issues/2
*pgreyson opened this issue November 07, 2011*
commit:
[041490b|https://github.com/callback/callback-weinre/commit/041490bdae1121de0328f2933836ab27a8e0beba]
With the code here:
http://pastebin.com/qHRkh1FF
The click handler still executes after removeEventListener is called.
Modifying ExceptionalCallbacks.coffee as follows fixes the problem (but
disables debugging of event callbacks)
addHookEventListener HookSites.Node_addEventListener, callSite_nodeAEL
*pmuellr commented November 08, 2011*
Thanks for the bug report and the sim
ple test case. But I don't think the referenced commit has anything to do with
the bug, does it?
*pgreyson commented November 08, 2011*
The referenced commit is just to identify the snapshot I'm using.
*pmuellr commented November 09, 2011*
Took a quick look this afternoon. It appears that I'll need to override
removeEventListener - seems like a stupid oversight on my part, sorry. Won't
really be able to do anything about it till I get back from vacation on 11/14.
In terms of the fix, first thing to try is to attach a new object to the
function the user passes in, which can be used to obtain the actual function to
pass into REL. Not happy about having to do this (extend user functions), but
don't see another obvious solution.
*pgreyson commented November 09, 2011*
I think that's right. I wonder though whether this is weinre trying to do too
much. I love having the remote javascript console and the DOM inspector. But
there's a limit to what you can do without having real debugger hooks and
seeing weinre hooking addEventListener gave me pause because it's changing
program behavior. I'd rather do that kind of thing via my own wrapper functions
which will be on whether weinre is there or not.
*pmuellr commented November 09, 2011*
absolutely - it's a bit of a dilemma - folks actually expect that weinre can do
this sort of thing, and so it's of course nice to be able to provide the
functionality. Unfortunately, it's mucking with your runtime more than you
would want it to, at least in this case. I have less hesitation adding
properties to DOM object, for instance.
And in most cases, the things weinre does probably doesn't cause any harm.
Still, kinda tending to agree with you that perhaps this should be moved to a
separate diagnostic library - actually, I prototyped it a bit here -
https://github.com/pmuellr/log-callback-error - and then folks can just add
that if they need it.
Another option would to make it - somehow - optional.
Got some time to noodle about it before next week ... :-)
*pmuellr commented November 14, 2011*
So, my current thinking is to:
* make the event handling callback checking code optional
* turn it on by default
One of the requirements will then be a way of toggling it off. Smells like a
requirement for a "settings" page. Don't have one right now, but this raises a
bunch of other issues - are "settings" saved on the server, in the debug
client, the debug target, or (oh noes) some combination of the three.
Pretend like we have a sane story for options :-) Do you think enabling this by
default is ok?
This is veering off-topic at this point, prolly better to create a new issue to
track it, and use this one to get the actual problem fixed ASAP.
*pgreyson commented November 14, 2011*
My preference would be to have it off by default. But that's because I would
never turn it on because I think that exception handling should be part of the
application code.
For storing settings, localStorage on the debug target could be a good option.
*pmuellr commented November 14, 2011*
OTOH, I've had feedback from people who assume that weinre provides this type
of support; and are surprised that it didn't (in earlier versions).
Kinda depends on the experience level of the developer I guess. And I happen to
think there are far fewer experts, such as yourself, than there are
non-experts, using weinre.
*pgreyson commented November 14, 2011*
Well you did ask :)
I can understand people wanting to have Weinre act like a full debugging
console and I guess catching exceptions in the event dispatcher is a reasonable
second best.
On the other hand, I think it could be very confusing. e.g., if you keep the
Weinre server process running but don't have the weinre console up then Weinre
could be catching and recovering from exceptions that will kill your app when
you turn the server off.
*pmuellr commented November 14, 2011*
weinre should not be "recovering" from exceptions at all. At the end, I
[rethrow the
exception|https://github.com/callback/callback-weinre/blob/master/weinre.web/modules/weinre/target/ExceptionalCallbacks.coffee#L47].
Which itself is not great - this re-roots the origin of the exception to my
exception handling code, instead of the real code. But since the runtime
doesn't really do anything interesting, right now, with the origin of the
exception, it shouldn't be hurting anything.
All the more reason to make this optional, BTW. Never know, some platform may
actually add decent callback exception handling to the runtime at any point in
time, and folks would want to disable this without waiting for a new weinre
release.
*pgreyson commented November 14, 2011*
Oh sorry I didn't notice that you re-throw. That all seems fine then.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
