incubator-callback-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Patrick Mueller (Created) (JIRA)" <j...@apache.org>
Subject [jira] [Created] (CB-82) removeEventListener broken
Date Tue, 29 Nov 2011 16:51:40 GMT
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

        

Mime
View raw message