tooltip race condition. throws TypeError: Cannot read property 'css' of null

[brenovieira]

Tooltip directive has a race condition on this line and this line.

If you have two tooltips and hover from one element to another a few times you will see the error.

I've created this plunker.

And attached these two screenshots to show you the error:

I tested with tooltip and tooltip-html and both have the same issue.

var tooltip;
...
if (!tooltip || !tooltip.html()) { return; }
if (!positionTimeout) {
   positionTimeout = $timeout(function() {
      // Reset the positioning.
      tooltip.css({ top: 0, left: 0 }); // throws TypeError
...

You're checking tooltip is null or undefined and setting a timeout, but if before the timeout triggers, removeTooltip() is called, you set tooltip = null; here and the exception occurs after timeout triggers.

It doesn't break the ui, but I've setted $exceptionhandler in my app to send me an email with the error, and I'm getting this error a lot (like 50 times yesterday), so I think it happens quite frequently in a normal use case with "common users".

A "double-checked locking" would solve the error, I mean, just check if (!tooltip || !tooltip.html()) { return; } again inside the timeout, before set position.

If you agree, I can open a PR with this.

[wesleycho]

@RobJacobs thoughts on this? Seems a bit fishy that this can even occur looking at the code, but I was able to reproduce this in the Plunker (with a lot of trouble).

[Foxandxss]

I reproduced it too. I know the feel because this kind of libraries are really prompted to race conditions.

[RobJacobs]

@brenovieira @wesleycho Here is a plunk with a proposed fix - move the calls to cancelShow() and cancelHide() to the removeTooltip() function. Seems to fix the problem, if there are no objections to this approach I will create a PR.

[brenovieira]

@RobJacobs , it seems to fix the issue.

But removeTooltip is called in just two places:

• on scope.$on('$destroy', ..) here
• on ttScope.$evalAsync(...) here either with animation or not.

on scope.$on(), we already call cancelShow() and cancelHide():

// Make sure tooltip is destroyed and removed.
scope.$on('$destroy', function onDestroyTooltip() {
  cancelShow();
  cancelHide();
  unregisterTriggers();
  removeTooltip();
  openedTooltips.remove(ttScope);
  ttScope = null;
});

Now, removeTooltip already calls unregisterObservers(), which is called twice on $on('$destroy'):

function removeTooltip() {
  unregisterObservers();
  transitionTimeout = null;
  ...

Adding cancelHide and cancelShow to removeTooltip solves the problem, but we will call twice even more methods and it is not good.

Also in removeTooltip we call transitionTimeout = null; without cancelling it as in cancelHide:

function cancelHide() {
  if (hideTimeout) {
    $timeout.cancel(hideTimeout);
    hideTimeout = null;
  }
  if (transitionTimeout) {
    $timeout.cancel(transitionTimeout);
    transitionTimeout = null;
  }
}

What do you think ?

[RobJacobs]

@brenovieira Don't confuse scope with the internal tooltip scope (tooltipLinkedScope). The scope.$on('$destroy is a fail safe cleanup that is called when the element that has the tooltip directive on it is destroyed. The tooltipLinkedScope is manually destroyed in removeTooltip(), scope.$on('$destroy' is not called when the tooltip hides.

scope.$on('$destroy does not call unregisterObservers() directly, it relies on that happening in the removeTooltip() function. It does however call unregisterTriggers(), perhaps you confused the 2?

The removeTooltip no longer needs to set the transitionTimeout to null as that will be done in the cancelHide(), I will remove that line.

Since the removeTooltip() now calls the cancelShow() and cancelHide(), they are no longer necessary in the scope.$on('$destroy'. I will remove those lines.

So now we end up with:

        function removeTooltip() {
          unregisterObservers();
          cancelShow();
          cancelHide();

          if (tooltip) {
            tooltip.remove();
            tooltip = null;
          }
          if (tooltipLinkedScope) {
            tooltipLinkedScope.$destroy();
            tooltipLinkedScope = null;
          }
        }

        scope.$on('$destroy', function onDestroyTooltip() {
          unregisterTriggers();
          removeTooltip();
          openedTooltips.remove(ttScope);
          ttScope = null;
        });

Typically we go through this sort of code analysis when we create the PR and can comment on the actual PR by line but I wanted to give you a chance to verify the proposed change fixed the issue you reported. So I'll go ahead and make a PR and we can continue the discussion there, thanks.

[brenovieira]

Don't confuse scope with the internal tooltip scope (tooltipLinkedScope). The scope.$on('$destroy is a fail safe cleanup that is called when the element that has the tooltip directive on it is destroyed. The tooltipLinkedScope is manually destroyed in removeTooltip(), scope.$on('$destroy' is not called when the tooltip hides.

Yeah, I'm not confusing it, I was just analizing the changes, but indeed we can discuss this on the PR.

scope.$on('$destroy does not call unregisterObservers() directly, it relies on that happening in the removeTooltip() function. It does however call unregisterTriggers(), perhaps you confused the 2?

That's true. My bad.

The removeTooltip no longer needs to set the transitionTimeout to null as that will be done in the cancelHide(), I will remove that line.

Great.

[progmars]

I got the same error in my project in "angular-bootstrap": "0.14.3" but my case is a bit different. I have a custom dropdown directive which has tooltip assigned through a function:

ng-attr-uib-tooltip="{{tooltipProperty ? getTooltip() : undefined}}"

If I use navigation key to go down through all items fast, tooltip is updated in rapid succession, maybe faster than it can deal with because if I hold down the key, navigation slows down and at one point I start getting errors:

TypeError: Cannot read property 'css' of null
    at http://mydomain/vendor/angular-bootstrap/ui-bootstrap-tpls.js:5033:26
    at http://mydomain/vendor/angular/angular.js:17855:31
    at completeOutstandingRequest (http://mydomain/vendor/angular/angular.js:5507:10)
    at http://mydomain/vendor/angular/angular.js:5784:7 undefined

[RobJacobs]

@progmars This fix is not in the 0.14.3 release. Do you still get the error when using the latest from the master branch?

[progmars]

I'm using bower version from https://github.com/angular-ui/bootstrap-bower , so it is tricky to upgrade tooltip to master because bower version has two large concatenated bootstrap files which cannot be found in this GitHub repository. I guess, I'll have to wait when bower repository will be updated.

[icfantv]

@progmars, you can build master yourself by pulling the master branch and running npm install followed by grunt. The resulting build files will be placed in the dist directory.