Closed
Bug 1099500
Opened 10 years ago
Closed 9 years ago
Potential memory leak in browser.js due to faulty toast notification handling
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox38 fixed, fennec+)
RESOLVED
FIXED
Firefox 38
People
(Reporter: wuhaoshu, Assigned: AndyP, Mentored)
References
Details
(Whiteboard: [lang=java][lang=js])
Attachments
(2 files)
1.41 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
3.35 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
OS: Mac OS X → Android
Hardware: x86 → All
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•10 years ago
|
Mentor: margaret.leibovic
tracking-fennec: ? → +
Updated•10 years ago
|
Whiteboard: [lang=java][lang=js]
Comment 1•10 years ago
|
||
Tiecheng are you interested in working on this bug?
Flags: needinfo?(wuhaoshu)
Reporter | ||
Comment 2•10 years ago
|
||
(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)
Updated•10 years ago
|
Assignee: nobody → sarentz
Updated•9 years ago
|
Assignee: sarentz → nobody
Assignee | ||
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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+
Comment 5•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d3ac91f56ef2
Assignee: nobody → drag
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d3ac91f56ef2
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Comment 7•9 years ago
|
||
The dependency bugs claim this broke super toast interactions (e.g, undo close tab, switch to tab).
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8565065 -
Attachment filename: bug1099500_PotentialMemoryLeakFix.diff → bug1099500_PotentialMemoryLeakFix.diff -v2
Assignee | ||
Updated•9 years ago
|
Attachment #8565065 -
Attachment description: bug1099500_PotentialMemoryLeakFix.diff → bug1099500_PotentialMemoryLeakFix.diff - v2
Attachment #8565065 -
Attachment filename: bug1099500_PotentialMemoryLeakFix.diff -v2 → bug1099500_PotentialMemoryLeakFix.diff
Comment 10•9 years ago
|
||
Any idea why the onToastHidden method is called for button clicks? I would have guessed the click handler would be called.
Assignee | ||
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
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+
Updated•9 years ago
|
Flags: needinfo?(margaret.leibovic)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•