Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: cordova-weinre
    • Labels:
      None

      Description

      from: https://github.com/callback/callback-weinre/issues/2

      pgreyson opened this issue November 07, 2011

      commit: 041490b

      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. 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.

        Activity

        Hide
        pmuellr Patrick Mueller added a comment -

        fixed in 1.6.1

        Show
        pmuellr Patrick Mueller added a comment - fixed in 1.6.1
        Hide
        pmuellr Patrick Mueller added a comment -

        fixed in version 1.6.1

        Show
        pmuellr Patrick Mueller added a comment - fixed in version 1.6.1
        Hide
        pmuellr Patrick Mueller added a comment -

        this fix has been shipped in 1.6.1

        Show
        pmuellr Patrick Mueller added a comment - this fix has been shipped in 1.6.1
        Hide
        pmuellr Patrick Mueller added a comment -

        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

        Show
        pmuellr Patrick Mueller added a comment - 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

          People

          • Assignee:
            pmuellr Patrick Mueller
            Reporter:
            pmuellr Patrick Mueller
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development