Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fixed] trying to remove listener where given id is undefined #9547

Merged
merged 16 commits into from Sep 11, 2019

Conversation

@mever
Copy link
Contributor

commented Jul 18, 2018

This PR fixes an exception where given argument to removeListenerById is undefined. I got this when trying to destroy a widget. With this fix all seems to work now.

oetiker added 2 commits Jul 18, 2018
@johnspackman

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

I'd like to know more about exactly what the cause or your original issue was - when I've come across this myself, it's always as a side effect of another problem (often to do with lists and controllers), and triggered by recursive binding (eg part way through binding a widget, an event causes the model or selection of the original widget to change, causing the binding to be redone part way through).

IMHO there are definitely opportunities to improve error detection in bindings because when it goes wrong it is hard to track down what on earth is really going on, but this also makes me cautious about applying fixes which may be only masking the "real" problem.

@mever

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

You are totally right. I am debugging now.

@mever

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

On line 257 of SingleValueBinding.js there is a similar check. When do the lengths of sources and listenerIds differ? When can I expect this to happen?

@johnspackman

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

Do you mean here ?

That looks like a redundant check, otherwise it would be a symptom of a resource leak (ie an un-freed listener) because it is not possible to release a listenerId without the object to release it from.

ie this code might be "more correct" (not that I'm suggesting that you make this change, but just to illustrate):

        for (var i = 0; i < sources.length; i++) {
          // check if a source is available
          if (listenerIds[i]) {
            if (qx.core.Environment.get("qx.debug")) {
              if (!sources[i]) {
                throw new Error("Lost source and cannot remove listener");
              }
            }
            sources[i].removeListenerById(listenerIds[i]);
          }
        }
@mever

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

Yes. Debugging the SingleValueBinding class is a real pain I must admit. My time is very limited so with some luck I can dig into it on Sunday when I continue.

oetiker added 2 commits Aug 22, 2018
@hkollmann

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

@mever: Did you have found some time to continue this issue?

@cajus
cajus approved these changes Sep 11, 2018
level420 added 4 commits Sep 17, 2018
@@ -1185,14 +1185,14 @@ qx.Class.define("qx.data.SingleValueBinding",
// go threw all added listeners (source)
for (var i = 0; i < id.sources.length; i++) {
// check if a source is available
if (id.sources[i]) {
if (id.sources[i] && id.listenerIds[i]) {

This comment has been minimized.

Copy link
@level420

level420 Nov 7, 2018

Member

@mever it would maybe help to add a this.error in the situation where somehow sources gets "out of sync" from listenerIds. we hesitate to merge this as the situation could be caused by a bug somewhere else which is then silenced by this pr. A this.error would help here as it offers a stack trace.

cajus added a commit that referenced this pull request Dec 18, 2018
Merge pull request #9611 from milandamen/chain-bind-remove-fix
Fixed removing listeners from binding chain with incomplete binding (see #9547)
level420 and others added 3 commits Feb 1, 2019
@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@mever could you update the PR as per @level420's suggestion? Thank you.

johnspackman added a commit to johnspackman/qooxdoo that referenced this pull request Jun 15, 2019
Merge commit '7cbcf73fe38e650daec33b639a028d9904b57a09'
* commit '7cbcf73fe38e650daec33b639a028d9904b57a09':
  Fix for qooxdoo#9620: Resizing a table column while any cell is being edited, resizes the editor even when in a different column.
  Keep qx from focussing non focusable widgets
  Rename methods to avoid name conflicts
  no message
  renames `objectId` and `owner` to `qxObjectId` and `qxOwner`; fixes qooxdoo#9612
  move application uitest to uitests/draganddrop
  Fixed removing listeners from binding chain with incomplete binding (see qooxdoo#9547)
  Allow not throwing an error when non-focusable widgets are focused/blurred
  fix broken unit test
  lint
  add support for child controls accessed via `#` fixes qooxdoo#9605
  adds event hooks to support recording of events for test script generation (use case for objectId)
  Fix documentation
  Allows to customize the label suffix (default " :") and the required suffix (default: red asterisk) in the form renderer by refining the values of two new properties.
  Fixing issue qooxdoo#9599 - width/height mixup in high-res images
  fix dragleave wrong target
johnspackman added a commit to johnspackman/qooxdoo that referenced this pull request Aug 14, 2019
Merge commit 'ed4601592a2d6256dcd0192d522a5692de768f7a'
* commit 'ed4601592a2d6256dcd0192d522a5692de768f7a':
  move application uitest to uitests/draganddrop
  Fixed removing listeners from binding chain with incomplete binding (see qooxdoo#9547)
  Allow not throwing an error when non-focusable widgets are focused/blurred
  fix broken unit test
  lint
  add support for child controls accessed via `#` fixes qooxdoo#9605
  adds event hooks to support recording of events for test script generation (use case for objectId)
  Fix documentation
  Allows to customize the label suffix (default " :") and the required suffix (default: red asterisk) in the form renderer by refining the values of two new properties.
  provides fixes for IDs when the parent ID changes, or when the parent is removed from the available IDs, the descendant ID also has to change.  Also further unit tests
  Fixing issue qooxdoo#9599 - width/height mixup in high-res images
  fix dragleave wrong target
  switch transform -> check
  lint
  fixes an issue with resolving IDs, where if the object does not have any owner its absolute ID will never be found, even if it has an ID and is registered as top level; also validation of ids and comment fixes
  fixes qooxdoo#9598
  fix theme switching
  fixes and corrections for PR
  Fix CheckGroupBox enabled consistency (closes qooxdoo#9350, refs qooxdoo#5519) (qooxdoo#9596)
  lint and version number
level420 added 3 commits Sep 11, 2019
@level420

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

merging. thank you @mever for the initial PR which was hijacked by me ;-)

@level420 level420 merged commit aee1cc4 into qooxdoo:master Sep 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pullapprove 1 group approved
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.