Closed Bug 1099500 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

33 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed
fennec + ---

People

(Reporter: wuhaoshu, Assigned: AndyP, Mentored)

References

Details

(Whiteboard: [lang=java][lang=js])

Attachments

(2 files)

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.
OS: Mac OS X → Android
Hardware: x86 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 1019735
tracking-fennec: --- → ?
Mentor: margaret.leibovic
tracking-fennec: ? → +
Whiteboard: [lang=java][lang=js]
Tiecheng are you interested in working on this bug?
Flags: needinfo?(wuhaoshu)
(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.
Flags: needinfo?(wuhaoshu)
Assignee: nobody → sarentz
Assignee: sarentz → nobody
The patch removes the if clause that prevented the release of callback data on ReasonHidden.TOUCH_OUTSIDE and ReasonHidden.REPLACED.
Attachment #8563491 - Flags: review?(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.
Attachment #8563491 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/d3ac91f56ef2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Depends on: 1133499
Depends on: 1133278
The dependency bugs claim this broke super toast interactions (e.g, undo close tab, switch to tab).
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.
This fixed the problems mentioned in bug #1133278 and bug #1133499 for me. Could someone else test it as well please?
Flags: needinfo?(margaret.leibovic)
Attachment #8565065 - Flags: review?(margaret.leibovic)
Attachment #8565065 - Attachment filename: bug1099500_PotentialMemoryLeakFix.diff → bug1099500_PotentialMemoryLeakFix.diff -v2
Attachment #8565065 - Attachment description: bug1099500_PotentialMemoryLeakFix.diff → bug1099500_PotentialMemoryLeakFix.diff - v2
Attachment #8565065 - Attachment filename: bug1099500_PotentialMemoryLeakFix.diff -v2 → bug1099500_PotentialMemoryLeakFix.diff
Any idea why the onToastHidden method is called for button clicks? I would have guessed the click handler would be called.
(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 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.
Attachment #8565065 - Flags: review?(margaret.leibovic) → review+
Depends on: 1134015
Flags: needinfo?(margaret.leibovic)
You need to log in before you can comment on or make changes to this bug.