1099500 - Potential memory leak in browser.js due to faulty toast notification handling

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tiecheng Wu]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0

Steps to reproduce:

Check the Firefox code snippets
---------------------
GeckoApp.java#840
                    public void onToastHidden(ButtonToast.ReasonHidden reason) {
                        if (reason == ButtonToast.ReasonHidden.TIMEOUT) {
                            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Toast:Hidden", buttonId));
                        }
                    }
---------------------

---------------------
browser.js#2159
    } else if (aTopic == "Toast:Hidden") {
      if (this.toast._callbacks[aData])
        delete this.toast._callbacks[aData];
---------------------

---------------------
ButtonToast.java#109
private void show(Toast t, boolean immediate) {
        // If we're already showing a toast, replace it with the new one by hiding the old one and quickly showing the new one.
        if (mCurrentToast != null) {
            hide(true, ReasonHidden.REPLACED);
            immediate = true;
        }
---------------------


---------------------
BrowserApp.java#2371
public boolean onInterceptTouchEvent(View view, MotionEvent event) {
            // Only try to hide the button toast if it's already inflated.
            if (mToast != null) {
                mToast.hide(false, ButtonToast.ReasonHidden.TOUCH_OUTSIDE);
            }
---------------------



Actual results:

GeckoApp.java notify browser.js to release the callback data, only when the Hidden reason is "ReasonHidden.TIMEOUT", and therefore in case the reasons are "ReasonHidden.TOUCH_OUTSIDE" or "ReasonHidden.REPLACED", callback data will just accumulate all the way.


Expected results:

In case of "ReasonHidden.TOUCH_OUTSIDE" and "ReasonHidden.REPLACED", the callback data should be released as well.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Tiecheng are you interested in working on this bug?

Comment 2

[Tiecheng Wu]

(In reply to Stefan Arentz [:st3fan] from comment #1)
> Tiecheng are you interested in working on this bug?

Sorry, I am more comfortable as a humble bug reporter. I checked the core product source by chance, while developing an extension for Fennec -> https://addons.mozilla.org/android/addon/smarter-scrolling/ , and I would love to contribute my thoughts, and join the discussion. But I think it is a right choice to let the experienced developer to actually address the bug, if it does exist.

Comment 3

[Andy Pusch [:AndyP]]

Attachment: bug1099500_PotentialMemoryLeakFix.diff

The patch removes the if clause that prevented the release of callback data on ReasonHidden.TOUCH_OUTSIDE and ReasonHidden.REPLACED.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8563491 [details] [diff] [review]
bug1099500_PotentialMemoryLeakFix.diff

Review of attachment 8563491 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

Looking through the code more closely, it looks like we never actually use this ReasonHidden for anything, and I think we should file a follow-up bug to just remove it altogether.

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/d3ac91f56ef2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3ac91f56ef2

Comment 7

[Aaron Train [:aaronmt]]

The dependency bugs claim this broke super toast interactions (e.g, undo close tab, switch to tab).

Comment 8

[Andy Pusch [:AndyP]]

I was able to reproduce the problem mentioned in bug #1133278. After unapplying the patch the problem was gone. I'm now trying to find out why exactly this happens.

Comment 9

[Andy Pusch [:AndyP]]

Attachment: bug1099500_PotentialMemoryLeakFix.diff - v2

This fixed the problems mentioned in bug #1133278 and bug #1133499 for me. Could someone else test it as well please?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Any idea why the onToastHidden method is called for button clicks? I would have guessed the click handler would be called.

Comment 11

[Andy Pusch [:AndyP]]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> Any idea why the onToastHidden method is called for button clicks? I would
> have guessed the click handler would be called.

ButtonToast.java hides the toast in its onClickListener using ReasonHidden.CLICKED. 
See: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ButtonToast.java#77

This seems to be a custom thing because apparently toast can not be clicked by default.
See: http://stackoverflow.com/questions/3308975/button-in-custom-android-toast

Comment 12

[:Margaret Leibovic]

Comment on attachment 8565065 [details] [diff] [review]
bug1099500_PotentialMemoryLeakFix.diff - v2

Review of attachment 8565065 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for being so quick to dig into this!

Can you please file a separate bug for this regression, and attach this patch there to land? As a general rule, we like to file new bugs for regressions, so that it's easier to track where various patches have landed.