Closed Bug 1134235 Opened 9 years ago Closed 9 years ago

Store Tab Queue urls in file

Categories

(Firefox for Android Graveyard :: General, defect)

37 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(1 file, 2 obsolete files)

Upon receiving a URL for addition to the tab queue, we need to add to a file.  This file then needs to be read when Fennec next loads.
Blocks: tab-queue
No longer depends on: tab-queue
Summary: Store Tab Queue urls in file and read on fennec load → Store Tab Queue urls in file
Narrowing the scope of this bug down to just writing the file for the time being as reading seems like it may be a large task just by itself
Attached patch Store Tab Queue urls in file (obsolete) — Splinter Review
Attachment #8566570 - Flags: review?(rnewman)
Blocks: 1129433
Comment on attachment 8566570 [details] [diff] [review]
Store Tab Queue urls in file

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

::: mobile/android/base/GeckoProfile.java
@@ +617,5 @@
>      }
>  
> +    public void writeFile(final String filename, final String data) {
> +        File file = new File(getDir(), filename);
> +        synchronized (this) {

Do you really want to block access to the GeckoProfile instance for three file ops, one of which is a write of unknown size?

What exactly is the critical section here, and what do you need to exclude? Are you trying to prevent races on file creation, races during write, or something else?

@@ +620,5 @@
> +        File file = new File(getDir(), filename);
> +        synchronized (this) {
> +            if (!file.exists()) {
> +                try {
> +                    file.createNewFile();

IIRC FileWriter (via FileOutputStream) creates the file for you, and throws if it can't create it (e.g., because it's a directory).

@@ +622,5 @@
> +            if (!file.exists()) {
> +                try {
> +                    file.createNewFile();
> +                } catch (IOException e) {
> +                    Log.e(LOGTAG, "Unable to create file", e);

You almost certainly want to give up if this fails, rather than roll on and try to write anyway. But removing the whole file creation block will solve that!

@@ +626,5 @@
> +                    Log.e(LOGTAG, "Unable to create file", e);
> +                }
> +            }
> +            try {
> +                //BufferedWriter for performance

Nit: space after //, full stop at end.

@@ +629,5 @@
> +            try {
> +                //BufferedWriter for performance
> +                BufferedWriter buf = new BufferedWriter(new FileWriter(file, false));
> +                buf.write(data);
> +                buf.close();

try..finally

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +20,5 @@
>      public static final long TOAST_TIMEOUT = 3000;
> +
> +    /**
> +     * Reads file and converts any content to JSON, adds passed in url to the data and writes back to the file,
> +     * creating the file if it doesn't already exist.  This should not be run on the main thread.

Add a ThreadUtils assertion to make sure we're not!

@@ +25,5 @@
> +     *
> +     * @param profile
> +     * @param url     URL to add
> +     */
> +    static public void queueUrl(GeckoProfile profile, String url) {

public static void queueURL(...)

@@ +31,5 @@
> +        String readingListContent = null;
> +        try {
> +            readingListContent = profile.readFile(FILE_NAME);
> +        } catch (IOException e) {
> +            e.printStackTrace();

Can we do better?

@@ +34,5 @@
> +        } catch (IOException e) {
> +            e.printStackTrace();
> +        }
> +
> +        JSONArray jsonArray = new JSONArray();

Don't bother with this duplicate init. Be more careful about the lifecycle and you won't need to do this at all.

@@ +39,5 @@
> +        if (!TextUtils.isEmpty(readingListContent)) {
> +            try {
> +                jsonArray = new JSONArray(readingListContent);
> +            } catch (JSONException e) {
> +                e.printStackTrace();

Same as above.

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +149,4 @@
>          final String args = intent.getStringExtra("args");
>          final String intentData = intent.getDataString();
>  
> +        // As we're doing disk IO, lets run this stuff in a separate thread.

let's

@@ +154,5 @@
> +            @Override
> +            public void run() {
> +                TabQueueHelper.queueUrl(GeckoProfile.get(getApplicationContext()), intentData);
> +            }
> +        }).start();

If this is a long-running service, you should probably be using a singleThreadExecutor instead of spawning a thread each time. It solves your locking problem, for one thing.
Attachment #8566570 - Flags: review?(rnewman)
Attached patch Store Tab Queue urls in file (obsolete) — Splinter Review
Have removed the blocking code from writeFile - this works for the current use case, but if this code is used elsewhere at some point then might we need to block against race conditions on read/write/delete?

Cleaned up the file creation code - cheers for that, much nicer now.

Added a ThreadExecutor and cleaned up nits. :)
Attachment #8566570 - Attachment is obsolete: true
Attachment #8571295 - Flags: feedback?(rnewman)
Comment on attachment 8571295 [details] [diff] [review]
Store Tab Queue urls in file

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

::: mobile/android/base/GeckoProfile.java
@@ +629,5 @@
> +                if (bufferedWriter != null) {
> +                    bufferedWriter.close();
> +                }
> +            } catch (IOException e) {
> +                Log.e(LOGTAG, "Error writing to file", e);

Error closing writer while writing to file

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +22,5 @@
>      public static final long TOAST_TIMEOUT = 3000;
> +
> +    /**
> +     * Reads file and converts any content to JSON, adds passed in url to the data and writes back to the file,
> +     * creating the file if it doesn't already exist.  This should not be run on the ui thread.

Nit: UI

@@ +27,5 @@
> +     *
> +     * @param profile
> +     * @param url     URL to add
> +     */
> +    public static void queueUrl(GeckoProfile profile, String url) {

s/Url/URL.

Take the filename as an argument to make testing easier?

@@ +34,5 @@
> +        String readingListContent = "";
> +        try {
> +            readingListContent = profile.readFile(FILE_NAME);
> +        } catch (IOException e) {
> +            Log.e(LOGTAG, "Error reading Tab Queue file contents.", e);

You're going to log this error whenever the file doesn't exist, which'll be the first time for every user.

Consider pushing this down into the profile? profile.readJSONArrayFromFile(FILE_NAME), maybe with a flag arg to default to an empty array?

@@ +38,5 @@
> +            Log.e(LOGTAG, "Error reading Tab Queue file contents.", e);
> +        }
> +        JSONArray jsonArray;
> +        try {
> +            jsonArray = new JSONArray(readingListContent);

Routinely this'll throw, because readingListContent = "".

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +54,4 @@
>      @Override
>      public void onCreate() {
>          super.onCreate();
> +        executorService = Executors.newSingleThreadExecutor();

I'm trying to verify the lifecycle of this, but it's hard when Bug 1130368 hasn't landed yet, so I can't see the current state of the code!

If you use an Executor, you should try to make sure you shut this down in the service's onDestroy.

But looking at the patch for Bug 1130368... given that:

* You've got a Handler wrapping a new thread
* You're handling intents
* This is a service

you should probably take a look at IntentService -- which is precisely a service that handles intents and dispatches work to a background thread -- think about START_FLAG_REDELIVERY, and try to fit into that lifecycle.

http://developer.android.com/reference/android/app/IntentService.html

It should turn this whole class into basically a single implementation of onHandleIntent.

@@ +143,4 @@
>          windowManager.removeView(layout);
>      }
>  
> +    private void addUrlToList(final Intent intentParam) {

Preference for URL here, too.
Attachment #8571295 - Flags: feedback?(rnewman) → feedback+
Have addressed nits - other comments below:

> Consider pushing this down into the profile?
> profile.readJSONArrayFromFile(FILE_NAME), maybe with a flag arg to default
> to an empty array?

Done with the exception that I've not added the extra arg as we don't need it currently - we can add that if we need it at a future date.


> ::: mobile/android/base/tabqueue/TabQueueService.java
> @@ +54,4 @@
> >      @Override
> >      public void onCreate() {
> >          super.onCreate();
> > +        executorService = Executors.newSingleThreadExecutor();
> 
> I'm trying to verify the lifecycle of this, but it's hard when Bug 1130368
> hasn't landed yet, so I can't see the current state of the code!
> 
> If you use an Executor, you should try to make sure you shut this down in
> the service's onDestroy.
> 
> But looking at the patch for Bug 1130368... given that:
> 
> * You've got a Handler wrapping a new thread
> * You're handling intents
> * This is a service
> 
> you should probably take a look at IntentService -- which is precisely a
> service that handles intents and dispatches work to a background thread --
> think about START_FLAG_REDELIVERY, and try to fit into that lifecycle.
> 
> http://developer.android.com/reference/android/app/IntentService.html
> 
> It should turn this whole class into basically a single implementation of
> onHandleIntent.

I had a play about with using an IntentService but there's fundamental issue which stops us from using it: the IntentService cannot run tasks in parallel. Hence all the consecutive intents will go into the message queue for the worker thread and will execute sequentially.  If we use an IntentService, each action has it's own separate lifecycle and spawns a new view, rather than being able to process the current task and then reuse the same view.

I've updated the service code in bug 1130368 to simplify a little but I think the approach is pretty solid now.
Attachment #8571295 - Attachment is obsolete: true
Attachment #8576049 - Flags: review?(rnewman)
I'm very frustrated at Gmail 'hiding' review requests. Please feel free to ping me on IRC if I take more than a day or two to respond to requests. My apologies.


> I had a play about with using an IntentService but there's fundamental issue
> which stops us from using it: the IntentService cannot run tasks in
> parallel. Hence all the consecutive intents will go into the message queue
> for the worker thread and will execute sequentially.  If we use an
> IntentService, each action has it's own separate lifecycle and spawns a new
> view, rather than being able to process the current task and then reuse the
> same view.
> 
> I've updated the service code in bug 1130368 to simplify a little but I
> think the approach is pretty solid now.

Gotcha. So you really have two task queues going here, and this is the second. I expect this'll work fine, but keep your eyes open for lifecycle issues (like the thread dying).
Status: NEW → ASSIGNED
Comment on attachment 8576049 [details] [diff] [review]
Store Tab Queue urls in file

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

::: mobile/android/base/GeckoProfile.java
@@ +618,5 @@
>          return null;
>      }
>  
> +    public void writeFile(final String filename, final String data) {
> +        File file = new File(getDir(), filename);

final

@@ +636,5 @@
> +            }
> +        }
> +    }
> +
> +    public JSONArray readJSONArrayFromFile(String filename) {

Maybe worth documenting that this never returns null.

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +179,5 @@
> +        // As we're doing disk IO, let's run this stuff in a separate thread.
> +        executorService.submit(new Runnable() {
> +            @Override
> +            public void run() {
> +                TabQueueHelper.queueURL(GeckoProfile.get(getApplicationContext()), intentData, filename);

Consider capturing the profile outside the runnable. If this takes a while to run, it's conceivable that unexpected things will happen.
Attachment #8576049 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/a6445426b812
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Blocks: 1145579
Blocks: 1145581
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: