Racy bookmark handling in Tab.java

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 12
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
Assignee: nobody → lucasr.at.mozilla
Priority: -- → P2
Hardware: All → ARM
(Assignee)

Comment 1

6 years ago
Created attachment 585693 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab
Attachment #585693 - Flags: review?(mark.finkle)
tracking-fennec: --- → 11+
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.
Attachment #585693 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
Created attachment 588845 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab

Added handles the case of cancelled AsyncTask.
Attachment #585693 - Attachment is obsolete: true
Attachment #588845 - Flags: review?(mark.finkle)
Attachment #588845 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 5

6 years ago
Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/82ee45c3759e
(Assignee)

Comment 6

6 years ago
Comment on attachment 588845 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab

Fix potential broken state on bookmarking UI.
Attachment #588845 - Flags: approval-mozilla-aurora?

Comment 7

6 years ago
Holding in the queue until this has had some time on m-c.
https://hg.mozilla.org/mozilla-central/rev/82ee45c3759e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12

Comment 9

6 years ago
Comment on attachment 588845 [details] [diff] [review]
Fix racy behaviour of bookmark checks in Tab

[Triage Comment]
Mobile only - approved for aurora.
Attachment #588845 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 10

6 years ago
Pushed to mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3b98406dfe0c
status-firefox11: --- → fixed
status-firefox12: --- → fixed
You need to log in before you can comment on or make changes to this bug.