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

fix memory leak in tooltip $element #19659

Closed
wants to merge 1 commit into from
Closed

fix memory leak in tooltip $element #19659

wants to merge 1 commit into from

Conversation

gregsheremeta
Copy link

Fixes #17973

@cvrebert cvrebert changed the title fix memory leak in tooltip $element. Fixes #17973 fix memory leak in tooltip $element Apr 1, 2016
@cvrebert
Copy link
Collaborator

cvrebert commented Apr 1, 2016

@twbs-savage Retry

@twbs-savage
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: a9efd66
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/120165154

(Please note that this is a fully automated comment.)

@cvrebert cvrebert added this to the v3.3.7 milestone Apr 1, 2016
@XhmikosR
Copy link
Member

@cvrebert: I think this looks good. Can we merge or is there something else?

@cvrebert
Copy link
Collaborator

@XhmikosR I just wasn't sure (without taking time to trace thru the flow) what situation makes the if necessary.

@gregsheremeta
Copy link
Author

I'm not sure about the "if" either. The fix seemed to work without that extra "if" in our project, but according to #16451, some people need the extra "if". So I added it in there. It's just extra safety -- I see no harm in leaving it.

@cvrebert
Copy link
Collaborator

Guess we should merge and add a TODO to revisit the "if" later.

@XhmikosR
Copy link
Member

@gregsheremeta: can you add the comment?

@cvrebert cvrebert closed this in f01f3e5 May 30, 2016
@mdo mdo mentioned this pull request May 30, 2016
@cvrebert
Copy link
Collaborator

Merged as f01f3e5. (Rebased to add the comment in.)

@cvrebert
Copy link
Collaborator

@gregsheremeta Thanks!

@cvrebert
Copy link
Collaborator

Bookkeeping note: No corresponding patch appears to be required for v4, since it already nullifies the element instance variable upon tooltip destruction:

this.element = null

@kntmrkm
Copy link

kntmrkm commented Jul 11, 2016

When release v3.3.7?

@cvrebert
Copy link
Collaborator

Some time in the next few weeks, most likely. As always, no absolute guarantee though.

@Herst
Copy link
Contributor

Herst commented Jul 26, 2016

The TODO comment is part of the release now.

@cvrebert
Copy link
Collaborator

That's fine.

@natalan
Copy link

natalan commented Sep 14, 2016

It looks like this PR is a root cause for #20511

@gregsheremeta
Copy link
Author

Probably want to add that "if" back in then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants