Last Comment Bug 704922 - Racy bookmark handling in Tab.java
: Racy bookmark handling in Tab.java
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: Firefox 12
Assigned To: Lucas Rocha (:lucasr)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-23 11:44 PST by Lucas Rocha (:lucasr)
Modified: 2012-01-19 03:56 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
Fix racy behaviour of bookmark checks in Tab (3.95 KB, patch)
2012-01-04 03:03 PST, Lucas Rocha (:lucasr)
mark.finkle: review-
Details | Diff | Review
Fix racy behaviour of bookmark checks in Tab (4.06 KB, patch)
2012-01-16 04:35 PST, Lucas Rocha (:lucasr)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Lucas Rocha (:lucasr) 2011-11-23 11:44:20 PST
When Tab URL changes, an AsyncTask is executed to update the bookmark state for the new location. However, the URL for the given tab might change before the AsyncTask ends, in which case we want to cancel any pending bookmark tasks and ignore their result.
Comment 1 Lucas Rocha (:lucasr) 2012-01-04 03:03:59 PST
Created attachment 585693 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-14 21:57:18 PST
Comment on attachment 585693 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab


>+        protected void onPostExecute(final Boolean isBookmark) {
>+            GeckoApp.mAppContext.runOnUiThread(new Runnable() {
>+                public void run() {
>+                    // Ignore this task if it's not about the current
>+                    // tab URL anymore.
>+                    if (!mUrl.equals(getURL()))
>+                        return;
>+
>+                    mCheckBookmarkTask = null;
>+                    setBookmark(isBookmark.booleanValue());
>+                }
>+            });

Should we set mCheckBookmarkTask to null no matter what? If not, won't we try to cancel it the next time?

r- to address the question, otherwise it looks fine.

I do wonder if we could add "cancel" to GeckoAsyncTask, making it more useful.
Comment 3 Lucas Rocha (:lucasr) 2012-01-16 04:24:20 PST
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 585693 [details] [diff] [review]
> Fix racy behaviour of bookmark checks in Tab
> 
> 
> >+        protected void onPostExecute(final Boolean isBookmark) {
> >+            GeckoApp.mAppContext.runOnUiThread(new Runnable() {
> >+                public void run() {
> >+                    // Ignore this task if it's not about the current
> >+                    // tab URL anymore.
> >+                    if (!mUrl.equals(getURL()))
> >+                        return;
> >+
> >+                    mCheckBookmarkTask = null;
> >+                    setBookmark(isBookmark.booleanValue());
> >+                }
> >+            });
> 
> Should we set mCheckBookmarkTask to null no matter what? If not, won't we
> try to cancel it the next time?

Good catch. Submitting a new patch now.

> r- to address the question, otherwise it looks fine.
> 
> I do wonder if we could add "cancel" to GeckoAsyncTask, making it more
> useful.

I actually think we shouldn't be using GeckoAsyncTask at all. We should simply add an util api that guarantees that AsyncTask's callbacks are called on the right threads.
Comment 4 Lucas Rocha (:lucasr) 2012-01-16 04:35:50 PST
Created attachment 588845 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab

Added handles the case of cancelled AsyncTask.
Comment 5 Lucas Rocha (:lucasr) 2012-01-16 06:58:46 PST
Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/82ee45c3759e
Comment 6 Lucas Rocha (:lucasr) 2012-01-16 06:59:37 PST
Comment on attachment 588845 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab

Fix potential broken state on bookmarking UI.
Comment 7 Alex Keybl [:akeybl] 2012-01-16 13:00:17 PST
Holding in the queue until this has had some time on m-c.
Comment 8 Justin Wood (:Callek) 2012-01-16 19:43:10 PST
https://hg.mozilla.org/mozilla-central/rev/82ee45c3759e
Comment 9 Alex Keybl [:akeybl] 2012-01-18 08:25:39 PST
Comment on attachment 588845 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab

[Triage Comment]
Mobile only - approved for aurora.
Comment 10 Lucas Rocha (:lucasr) 2012-01-18 08:56:42 PST
Pushed to mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3b98406dfe0c

Note You need to log in before you can comment on or make changes to this bug.