Closed Bug 746132 Opened 12 years ago Closed 12 years ago

Screenshot buffers won't be freed if tab not found

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: blassey, Assigned: blassey)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
      No description provided.
Attachment #615695 - Flags: review?(bugmail.mozilla)
Comment on attachment 615695 [details] [diff] [review]
patch

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

Why do we call freeDirectBuffer from java rather than just doing it in the c++ code? Is is to reduce memory pressure before calling processThumbnail?

Also, it might be worthwhile wrapping that entire function in a try/finally and doing the freeDirectBuffer in the finally. That would also make sure to free the buffer if say Bitmap.createBitmap threw an exception, or if future changes to the code added another early-exit path.
Attachment #615695 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (:kats) from comment #1)
> Comment on attachment 615695 [details] [diff] [review]
> patch
> 
> Review of attachment 615695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we call freeDirectBuffer from java rather than just doing it in the
> c++ code? Is is to reduce memory pressure before calling processThumbnail?
this is not freed from the C++ code because it gets processed asynchronously in a runnable (i.e. after the free would happen).

> 
> Also, it might be worthwhile wrapping that entire function in a try/finally
> and doing the freeDirectBuffer in the finally. That would also make sure to
> free the buffer if say Bitmap.createBitmap threw an exception, or if future
> changes to the code added another early-exit path.
This is probably best
Attached patch patchSplinter Review
Assignee: nobody → blassey.bugs
Attachment #615695 - Attachment is obsolete: true
Attachment #617095 - Flags: review?(bugmail.mozilla)
Attachment #617095 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 617095 [details] [diff] [review]
patch

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any): This prevents leaks in fennec native. Should be zero risk
Attachment #617095 - Flags: approval-mozilla-central?
Attachment #617095 - Flags: approval-mozilla-central? → approval-mozilla-central+
https://hg.mozilla.org/mozilla-central/rev/344a48cc9998
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Regression :( Robocop Checkerboarding Benchmark increase 21.8% on Android 2.2 (Native) Mozilla-Inbound
------------------------------------------------------------------------------------------------------
   Previous: avg 0.813 stddev 0.006 of 30 runs up to revision d2b34e4338bb
   New     : avg 0.990 stddev 0.000 of 5 runs since revision 344a48cc9998
   Change  : +0.177 (21.8% / z=27.800)
   Graph   : http://mzl.la/I4Nh5G

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d2b34e4338bb&tochange=344a48cc9998

Changesets:
 * http://hg.mozilla.org/integration/mozilla-inbound/rev/190fc7cd65c6
   : George Wright <gwright@mozilla.com> - Bug 747274 - Add a pref (default to true on Android) to forcibly use nearest pixel filtering for background drawing. r=jrmuizel,ajuma a=blassey
   : http://bugzilla.mozilla.org/show_bug.cgi?id=747274

 * http://hg.mozilla.org/integration/mozilla-inbound/rev/9b2440c92909
   : Brad Lassey <blassey@mozilla.com> - bug 746139 - local JNI frame won't be popped if init fails r=dougt a=lsblakk
   : http://bugzilla.mozilla.org/show_bug.cgi?id=746139

 * http://hg.mozilla.org/integration/mozilla-inbound/rev/344a48cc9998
   : Brad Lassey <blassey@mozilla.com> - bug 746132 - Screenshot buffers won't be freed if tab not found r=kats a=lsblakk
   : http://bugzilla.mozilla.org/show_bug.cgi?id=746132
(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Regression :( Robocop Checkerboarding Benchmark increase 21.8% on Android
> 2.2 (Native) Mozilla-Inbound
> -----------------------------------------------------------------------------
> -------------------------
>    Previous: avg 0.813 stddev 0.006 of 30 runs up to revision d2b34e4338bb
>    New     : avg 0.990 stddev 0.000 of 5 runs since revision 344a48cc9998
>    Change  : +0.177 (21.8% / z=27.800)
>    Graph   : http://mzl.la/I4Nh5G

Talos might not know it, but "bigger is better" for the checkerboarding tests.
OK, filed bug 748174.  Thanks!
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.