704922 - Racy bookmark handling in Tab.java

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Lucas Rocha (:lucasr)]

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)]

Attachment: Fix racy behaviour of bookmark checks in Tab

Comment 2

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

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)]

(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)]

Attachment: Fix racy behaviour of bookmark checks in Tab

Added handles the case of cancelled AsyncTask.

Comment 5

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/82ee45c3759e

Comment 6

[Lucas Rocha (:lucasr)]

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]]

Holding in the queue until this has had some time on m-c.

Comment 8

[Justin Wood (:Callek)]

https://hg.mozilla.org/mozilla-central/rev/82ee45c3759e

Comment 9

[Alex Keybl [:akeybl]]

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)]

Pushed to mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3b98406dfe0c