You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by "Patrick Mueller (Created) (JIRA)" <ji...@apache.org> on 2011/11/29 17:51:40 UTC
[jira] [Created] (CB-82) removeEventListener broken
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
[jira] [Closed] (CB-82) removeEventListener broken
Posted by "Patrick Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-82?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Mueller closed CB-82.
-----------------------------
fixed in 1.6.1
> 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
> Priority: Critical
>
> 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
[jira] [Commented] (CB-82) removeEventListener broken
Posted by "Patrick Mueller (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-82?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13161145#comment-13161145 ]
Patrick Mueller commented on CB-82:
-----------------------------------
this fix has been shipped in 1.6.1
> 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
> Priority: Critical
>
> 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
[jira] [Updated] (CB-82) removeEventListener broken
Posted by "Patrick Mueller (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-82?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Mueller updated CB-82:
------------------------------
Priority: Critical (was: Major)
> 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
> Priority: Critical
>
> 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
[jira] [Resolved] (CB-82) removeEventListener broken
Posted by "Patrick Mueller (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-82?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Patrick Mueller resolved CB-82.
-------------------------------
Resolution: Fixed
fixed in version 1.6.1
> 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
> Priority: Critical
>
> 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
[jira] [Commented] (CB-82) removeEventListener broken
Posted by "Patrick Mueller (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/CB-82?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13160300#comment-13160300 ]
Patrick Mueller commented on CB-82:
-----------------------------------
I'ved decided to just remove the handler for exceptions in callbacks. The proper fix would require adding properties to the original callback to reference the wrappered callback. Seems tame enough, but also seems like it could easily be the source for difficult-to-diagnose errors.
I think the thing to do is to continue working on a separate library to handle this, and once it's been battle-proven, we could look at adding it to weinre.
The changeset to remove this code is here:
https://github.com/callback/callback-weinre/commit/9cc3cf33c2e929a6b077b229fc0c4ce2c3367dbc
> 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
> Priority: Critical
>
> 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