730039 - Content provider for Fennec's open tabs

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

+++ This bug was initially created as a clone of Bug #715644 +++

...

Fennec's own current tabs are the second part of this. In order to support syncing of Fennec's open tabs, we need a content provider to:

  * Retrieve the current set of open tabs, ideally by modified time, so as to avoid uploading *all the tabs* on every sync
  * For each tab, retrieve the four values above.

You get extra points if you design this in a way that's flexible for future extension (Panorama groups, for example).

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

This patch does the initial commit for syncing Fennec tabs with the database for the Sync to take it.

I have few doubts:
1. I am not sure of the name "TabsAccessor". I was using a name similar to how Sync uses. We might need a new name. ;)
2. The variable is in Tabs.java as only Tabs.java and Tab.java are accessing it currently. Is this fine?
3. History will be added whenever there is a "SessionHistory:New" event from Gecko. How should we react for "Back" and "Forward" operations? And do we store the "position" in the history-array in the db as well? (Like I navigate 4 pages and go back twice, do we store a -2 or something?)
4. I am not sure when we need to update LAST_USED field. Do we do it for any change in any of the other fields? Or how does it work? If LAST_USED timestamp takes care of the position of the tab (like which is foreground and which is background), does position actually help?
5. I am calling the "stack" of tabs as "position" in Tabs.java. This corresponds to POSITION in database. Can we use a better name? stack probably? stackPosition?

This just makes quite a lot of db calls. I am a bit scared. 
Every change in a tab does updating that particular field for tab.
A tab selection will cause changing the position of all tabs in front of it in the "position" stack.
A navigation will write the new history by forming a JSON array and converting it to a string.

Additionally, all these operations will update the clients tab for last_modified timestamp.

Probably I should look into history/bookmarks to console myself :(

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 601474 [details] [diff] [review]
WIP

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

A good start, but lots of concurrency and OO issues. See comments!

::: mobile/android/base/Tab.java
@@ +68,5 @@
>      private static float sDensity = 1;
>      private static int sMinScreenshotWidth = 0;
>      private static int sMinScreenshotHeight = 0;
>      private int mId;
> +    private long mSyncId;

This seems like a misnomer. Perhaps "tabRowID"? (Sorry, in Fennec Microsoftese: mlTabRowId. ;))

@@ +283,5 @@
>      public boolean isExternal() {
>          return mExternal;
>      }
>  
> +    public void setSyncId(long syncId) {

When you see a non-atomic field (and longs aren't atomic), and a setter without any form of synchronization, you should start screaming until all the spiders in the room scuttle away in horror.

All Java code is multithreaded code.

@@ +495,5 @@
>                  mHistory.remove(mHistoryIndex);
>              }
>              HistoryEntry he = new HistoryEntry(uri, "");
>              mHistory.add(he);
> +            updateHistoryInSync();

I doubt that you want to do that here.

My inclination is to make Tab thread-safe (which it probably should be anyway), and then have a worker queue of tabs that a background task can process periodically.

That is, when a tab changes you simply push its ID into a queue, and continue on your merry high-performance way. A task is blocking on that queue, and will take care of actually updating the DB.

Note that if lots of tabs change at once, the task can grab multiple IDs at once, and minimize the number of DB hits.

::: mobile/android/base/Tabs.java
@@ +97,5 @@
>          tabs.put(id, tab);
>          order.add(tab);
> +        position.add(tab);
> +        
> +        Uri uri = Tabs.mSyncAccessor.insertTab(title, url, System.currentTimeMillis(), getPositionOf(tab));

Failure of OO here.

Note that you have a Tabs object, poking directly at some database wrapper with the inner fields of a Tab object, introspecting the response, and then setting some tracking value.

Why doesn't a Tab know how to persist itself to the DB? That is,

  tab.persist(Tabs.mSyncAccessor, getPositionOf(tab));

That way Tab can set its own "syncId", and Tabs doesn't need to worry about all that crap. And Tab is free to do stuff that doesn't impact performance, like enqueue itself for writing into the DB, rather than being forced to do it synchronously here.

@@ +101,5 @@
> +        Uri uri = Tabs.mSyncAccessor.insertTab(title, url, System.currentTimeMillis(), getPositionOf(tab));
> +        if (uri != null) {
> +            long syncId = ContentUris.parseId(uri);
> +            if (syncId != -1)
> +                tab.setSyncId(syncId);

Bug lurking here. What if syncId is -1? The tab doesn't get a syncId.

@@ +120,5 @@
>      public void removeTab(int id) {
>          if (tabs.containsKey(id)) {
> +            Tab tab = getTab(id);
> +            order.remove(tab);
> +            position.remove(tab);

You have an awful race condition hiding here, no? There's code all over this class that's modifying two unsynchonized ArrayLists and a HashMap. Oh god.

Note that non-synchronized concurrent modification of even a single ArrayList is a no-no.

@@ +125,2 @@
>              tabs.remove(id);
> +            Tabs.mSyncAccessor.deleteTab(tab.getSyncId());

Again, this should be the job of the Tab.

  tab.persistRemoval(mSyncAccessor);

@@ +143,5 @@
>              GeckoApp.mAppContext.showAboutHome();
>          else
>              GeckoApp.mAppContext.hideAboutHome();
>  
> +        // Update the position of all the tabs until the selected tab in Sync.

Not in Sync; in our tabs content provider.

@@ +146,5 @@
>  
> +        // Update the position of all the tabs until the selected tab in Sync.
> +        int tabPosition = getPositionOf(tab);
> +        position.remove(tab);
> +        position.add(0, tab);

This is expensive; that 'remove' will reposition every item in the array, and so will the 'add'!

You might want to consider a different data structure.

@@ +149,5 @@
> +        position.remove(tab);
> +        position.add(0, tab);
> +
> +        for (int i = 0; i <= tabPosition; i++) {
> +            Tabs.mSyncAccessor.updatePosition(position.get(i).getSyncId(), i);

This is the one instance in which I think Tabs should be responsible for updating the DB, and it should do it like this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#1246

You could also be very clever with arithmetic, using two IN statements and an UPDATE which increments or decrements positions. I leave that as an exercise for the reader.

@@ +181,5 @@
>      }
>  
> +    public int getPositionOf(Tab tab) {
> +        return position.lastIndexOf(tab);
> +    }

OK, you'll have to clarify here what you need `position` for. Why is `order` not sufficient?

::: mobile/android/base/TabsAccessor.java
@@ +28,5 @@
> +import android.os.Build;
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public class TabsAccessor {

You definitely, definitely want to use this off the main thread.

@@ +35,5 @@
> +
> +    static final String TABLE_TABS = "tabs";
> +    static final String TABLE_CLIENTS = "clients";
> +
> +    static final String FENNEC_GUID = "NULL";

This can go.

@@ +42,5 @@
> +
> +    public TabsAccessor(Context context) {
> +        mResolver = context.getContentResolver();
> +
> +        // Add the fennec client if it is not added yet

Capitalization and punctuation.

@@ +47,5 @@
> +        Cursor fennecAdded = isFennecClientAdded();
> +        if (fennecAdded.getCount() == 0)
> +            addFennecClient();
> +
> +        fennecAdded.close();

if (!hasFennecClientRecord()) {
  addFennecClient();
}

See below.

HOWEVER: you have a race condition here. Two simultaneous TabsAccessor constructors can result in a DB with multiple Fennec client records. You need to extend your ContentProvider to do the insertion on your behalf, using CONFLICT_IGNORE, because ContentResolver doesn't offer any kind of useful synchronization or transactionality.

Alternatively you need to ensure that there's only a single instance of TabsAccessor (with a correctly implemented singleton), or a single instance of the ContentResolver (which is harder).

@@ +69,5 @@
> +
> +    // Client related methods
> +    Uri addFennecClient() {
> +        ContentValues values = new ContentValues();
> +        values.put(BrowserContract.Tabs.CLIENT_GUID, FENNEC_GUID);

Incorrect.

  values.putNull(BrowserContract.Tabs.CLIENT_GUID);

@@ +70,5 @@
> +    // Client related methods
> +    Uri addFennecClient() {
> +        ContentValues values = new ContentValues();
> +        values.put(BrowserContract.Tabs.CLIENT_GUID, FENNEC_GUID);
> +        values.put(BrowserContract.Tabs.CLIENT_NAME, "this-fennec");

You can leave that null or empty, too.

@@ +97,5 @@
> +        return mResolver.query(getClientsUri(),
> +                               new String[] { BrowserContract.Tabs.CLIENT_GUID },
> +                               selectColumn(TABLE_CLIENTS, BrowserContract.Tabs.CLIENT_GUID),
> +                               mSelectionArgs,
> +                               TABLE_CLIENTS + "." + BrowserContract.Tabs.CLIENT_GUID + " DESC");

private static final String[] GUID_COLS  = new String[] { BrowserContract.Tabs.CLIENT_GUID };
private static final String GUID_IS_NULL = BrowserContract.Tabs.CLIENT_GUID + " IS NULL";

protected boolean cursorIsEmpty(Cursor c) {
  if (c == null)
    return true;

  try {
    return !c.moveToNext();
  } finally {
    c.close();
  }
}
public boolean hasFennecClientRecord() {
  return !cursorIsEmpty(mResolver.query(getClientsUri(), GUID_COLS, GUID_IS_NULL, null, null));
}

Comment 3

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #1)

> I have few doubts:
> 1. I am not sure of the name "TabsAccessor". I was using a name similar to
> how Sync uses. We might need a new name. ;)

TabsStorage?

> 2. The variable is in Tabs.java as only Tabs.java and Tab.java are accessing
> it currently. Is this fine?

Variable?

> 3. History will be added whenever there is a "SessionHistory:New" event from
> Gecko. How should we react for "Back" and "Forward" operations? And do we
> store the "position" in the history-array in the db as well? (Like I
> navigate 4 pages and go back twice, do we store a -2 or something?)

I realize I wasn't particularly clear about what "position" means, so to avoid ambiguity:

The tabs retrieved from and stored to the Sync server are an ordered array.

The tabs stored in the database are not.

Consequently, we add a "position" column to the database to allow us to maintain some order.

We don't need to store an index into the history array; just truncate it so that the current page is at index 0, and the past increases from there.


> 4. I am not sure when we need to update LAST_USED field. Do we do it for any
> change in any of the other fields? Or how does it work? If LAST_USED
> timestamp takes care of the position of the tab (like which is foreground
> and which is background), does position actually help?

Position and last used provide two ways to order the same data.

Last used should be updated whenever you switch tabs or navigate. It only needs to be flushed to the database at significant intervals. This is all eminently lossy data.


> 5. I am calling the "stack" of tabs as "position" in Tabs.java. This
> corresponds to POSITION in database. Can we use a better name? stack
> probably? stackPosition?

There's already an "order" value. Why do you need position?


> This just makes quite a lot of db calls. I am a bit scared. 
> Every change in a tab does updating that particular field for tab.
> A tab selection will cause changing the position of all tabs in front of it
> in the "position" stack.
> A navigation will write the new history by forming a JSON array and
> converting it to a string.
> 
> Additionally, all these operations will update the clients tab for
> last_modified timestamp.
> 
> Probably I should look into history/bookmarks to console myself :(

Firstly, don't write on every change.

Secondly, don't write every value as it changes.

You can mark tabs as dirty and periodically flush them to disk on a background thread, perhaps in an onPause handler, when a tab is closed, and otherwise every 30 seconds if anything has changed.


This can be best-effort; it doesn't have to be real-time.

Comment 4

[Richard Newman [:rnewman]]

A further alternative: dump the whole idea of updating the DB.

Fennec has a practical limit on how many tabs it can have open, right?

On some event (timer, closes, onPause, whatever) just delete all of Fennec's rows from the DB and then batchInsert a bunch of new ones.

I'd suggest implementing both approaches (background updates, batch wipe/re-add) and see what happens. batchInsert should occur in one transaction, which will sure beat the sequence of updates that occurs now, but real world numbers trump speculation.

Comment 5

[Sriram Ramasubramanian [:sriram]]

The "order" list is for holding the "order in which tabs were added". HashMap cannot maintain this "order".
The "position" list is for holding the tabs in "most recently used" order.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> The "order" list is for holding the "order in which tabs were added".
> HashMap cannot maintain this "order".
> The "position" list is for holding the tabs in "most recently used" order.

I'm not sure I get why you need both of these. Is one used to compute where you go when you close a tab, and one the displayed order? If so, that makes sense.

If so, I'd suggest that "order" is what I'd call a "tabStack" (because you push tabs as they're added, and the sequence doesn't change), and "position" is what I'd call "tabSequence".

Comment 7

[Sriram Ramasubramanian [:sriram]]

I agree that "order" is "tabStack" as tab get added to it one after the other. (Or probably a queue). This is used to show the tabs-list in the order the user opened them.

"position" holds the sequence of "most recently used". I prefer calling it position so that it corresponds with what is in database.

So, order - "order in which tab added by user"
position - "position in MRU list"

Also, I felt ArrayList<?> is a good datastructure for position. Probably a "generic" List<?> is better? [Coming from 3 yrs of C/C++ background, I still think of lists in terms of pointers :( ]

Comment 8

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> I agree that "order" is "tabStack" as tab get added to it one after the
> other. (Or probably a queue). This is used to show the tabs-list in the
> order the user opened them.
> 
> "position" holds the sequence of "most recently used". I prefer calling it
> position so that it corresponds with what is in database.

The position we want in the database is the displayed position, not the MRU; we can get the MRU stack implicitly by sorting on the lastUsed timestamp for each record.

If Fennec doesn't use the position in the MRU list itself, then you can kill that, because Sync doesn't want it. Write the stack order to the DB.


> Also, I felt ArrayList<?> is a good datastructure for position. Probably a
> "generic" List<?> is better? [Coming from 3 yrs of C/C++ background, I still
> think of lists in terms of pointers :( ]

Depends. If you want cheap deletion from the middle, or cheap addition to one or both ends, then LinkedList is a better choice. ArrayList will have to move items between array indices if you delete an item from the middle.

Comment 9

[Sriram Ramasubramanian [:sriram]]

> This seems like a misnomer. Perhaps "tabRowID"? (Sorry, in Fennec
> Microsoftese: mlTabRowId. ;))

Did you mean mTabRowId? :)

> > +    public void setSyncId(long syncId) {
> 
> When you see a non-atomic field (and longs aren't atomic), and a setter
> without any form of synchronization, you should start screaming until all
> the spiders in the room scuttle away in horror.
> 
> All Java code is multithreaded code.

I agree it's multithreaded. But this called only when a "tab" is "inserted" into DB. So, I don't see any threading issue. So does all other getter-setter in that file. :(

>   tab.persist(Tabs.mSyncAccessor, getPositionOf(tab));
> 
> That way Tab can set its own "syncId", and Tabs doesn't need to worry about
> all that crap. And Tab is free to do stuff that doesn't impact performance,
> like enqueue itself for writing into the DB, rather than being forced to do
> it synchronously here.

I agree with this. I felt it should be with tab. :)

> 
> @@ +101,5 @@
> > +        Uri uri = Tabs.mSyncAccessor.insertTab(title, url, System.currentTimeMillis(), getPositionOf(tab));
> > +        if (uri != null) {
> > +            long syncId = ContentUris.parseId(uri);
> > +            if (syncId != -1)
> > +                tab.setSyncId(syncId);
> 
> Bug lurking here. What if syncId is -1? The tab doesn't get a syncId.

I don't think that can happen. It can happen only when the row is not inserted in DB. Then something wrong is happening with DB! :(

> 
> @@ +120,5 @@
> >      public void removeTab(int id) {
> >          if (tabs.containsKey(id)) {
> > +            Tab tab = getTab(id);
> > +            order.remove(tab);
> > +            position.remove(tab);
> 
> You have an awful race condition hiding here, no? There's code all over this
> class that's modifying two unsynchonized ArrayLists and a HashMap. Oh god.
> 
> Note that non-synchronized concurrent modification of even a single
> ArrayList is a no-no.

It might seem so, but everything is checked at the point of calling. So far everything is working fine. :) List updates happen on add/select/close tabs. Everywhere else, just lookups happen. I can look into this sometime later though.

> 
> @@ +146,5 @@
> >  
> > +        // Update the position of all the tabs until the selected tab in Sync.
> > +        int tabPosition = getPositionOf(tab);
> > +        position.remove(tab);
> > +        position.add(0, tab);
> 
> This is expensive; that 'remove' will reposition every item in the array,
> and so will the 'add'!
> 
> You might want to consider a different data structure.

This wouldn't be needed anymore, as we aren't in need of position list.

> 
> @@ +47,5 @@
> > +        Cursor fennecAdded = isFennecClientAdded();
> > +        if (fennecAdded.getCount() == 0)
> > +            addFennecClient();
> > +
> > +        fennecAdded.close();
> 
> if (!hasFennecClientRecord()) {
>   addFennecClient();
> }
> 
> See below.
> 
> HOWEVER: you have a race condition here. Two simultaneous TabsAccessor
> constructors can result in a DB with multiple Fennec client records. You
> need to extend your ContentProvider to do the insertion on your behalf,
> using CONFLICT_IGNORE, because ContentResolver doesn't offer any kind of
> useful synchronization or transactionality.
> 
> Alternatively you need to ensure that there's only a single instance of
> TabsAccessor (with a correctly implemented singleton), or a single instance
> of the ContentResolver (which is harder).

So there is a "single" instance of TabsAccessor in Tabs.java (which is a singleton class). And this particular "insertion" check happens only when "Fennec starts". So, there is no race condition involved. I made sure to have it inside the constructor, just so that I don't want anyone else to touch it. This is a "static" variable in Tabs.java. I can make it final to make sure no one else changes it.

>   values.putNull(BrowserContract.Tabs.CLIENT_GUID);

Aah. I didn't know this existed. :(

Comment 10

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> > This seems like a misnomer. Perhaps "tabRowID"? (Sorry, in Fennec
> > Microsoftese: mlTabRowId. ;))
> 
> Did you mean mTabRowId? :)

No, that was a Hungarian Notation joke.

m = member
l = long


> > Bug lurking here. What if syncId is -1? The tab doesn't get a syncId.
> 
> I don't think that can happen. It can happen only when the row is not
> inserted in DB. Then something wrong is happening with DB! :(

So don't just ignore it: throw, choose a sane alternative value, or otherwise take action.


> > Note that non-synchronized concurrent modification of even a single
> > ArrayList is a no-no.
> 
> It might seem so, but everything is checked at the point of calling. So far
> everything is working fine. :)

The Java Memory Model disagrees with you. Srsly, unless you can *guarantee* that every method that might touch a member is going to be run in the same thread, you should ensure thread safety.


> Everywhere else, just lookups happen. I can look into this sometime later
> though.

"It is a common mistake to assume that synchronization needs to be used only when writing to shared variables; this is simply not true.

"For each mutable state variable that may be accessed by more than one thread, all accesses to that variable must be performed with the same lock held. In this case, we say that the variable is guarded by that lock."

  -- Bloch, Joshua; Goetz, Brian; Peierls, Tim; Bowbeer, Joseph; Holmes, David; Lea, Doug (2006-05-09). Java Concurrency in Practice (Kindle Locations 999-1000). Pearson Education (US). Kindle Edition. 

If you haven't read JCIP, you should take a week off work and devour it. It will save you three weeks in hard-to-find concurrency bugs.


> So there is a "single" instance of TabsAccessor in Tabs.java (which is a
> singleton class). And this particular "insertion" check happens only when
> "Fennec starts".

Still races: what if the ContentProvider is processing a wipe at the same time? Urk.

Comment 11

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

I am glad work is underway for this feature, but it's not a blocker for the release. Showing tabs from other computers is the blocker for now.

Given the fun work to tease apart the concurrency issues, holding this work for now seems like a better use of resources to get other blockers wrapped up first.

Comment 12

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

this does not block

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Lets discuss at the next triage

Comment 14

[Allison Naaktgeboren :ally]

If possible can we pick this back up for 16? Now that native is on beta, we've been hearing a lot of requests for the open tabs.

Comment 15

[:Margaret Leibovic]

I'll pick this up. Tracking 16 seems unrealistic at this point, but we could probably get something for 17.

Comment 16

[:Margaret Leibovic]

Attachment: rough WIP

A lot has changed since Sriram's initial patch, and I don't think we'll need to do quite as much work now. This is a very rough start at this, but I want to get some feedback before I do too much work :)

Things to note:
-I decided to just add the local fennec client to the db in the TabsProvider initialization, since we only ever need to add it once. I don't see why we'd delete it, and this gets rid of the racy isFennecClientAdded check.
-So far I just made insertLocalTab async, but I imagine we'll want all of these db operations to be done on background threads. Is there a better/more generic way to do this than making a listener interface for each method?
-I still need to figure out when we'll actually make a call to persist the tab data, and I was curious about the best way to implement a queue to try to batch these updates.
-I still need to add some functionality to Tab/Tabs to get values for the history/lastUsed/position columns, but this shouldn't be too hard. I think a tab should just keep track of all its own info, so I plan to keep track of these in Tab.

I also noticed that TabsProvider/TabsAccessor need some tests :(

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 646744 [details] [diff] [review]
rough WIP

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

We talked this through on Friday, but summary:

* Thumbs up on a fixed client record.
* Broadly speaking, I'd like to see `Tabs` manage Tab instances and DB flushing, which allows it to track positions in the DB without much wiring.
* Tabs can listen to change events from Tab instances, and flush out the tab set periodically (by tracking scores and changed tabs). I think this will meet our needs better than incrementally touching the DB, *unless* (or until) this DB can be used in place of whatever session store is already being used.
* … but either approach is fine: I just think periodic flushing is less likely to impact perf where we care about it.
* After some amount of change, and/or enough time has passed, you can request a sync for the tabs engine.

Comment 18

[:Margaret Leibovic]

Attachment: WIP

The functionality is mostly here, but I still need to write some tests. I'm also worried about the thread safety of persistAllTabs. I made it synchronized, but I don't think that would protect against mOrder changing on a different thread, would it?

I also wonder if we should make a call to persistAllTabs on shutdown, but I don't want to cause perf problems.

Comment 19

[:Margaret Leibovic]

I just realized I forgot to include the "request a sync for the tabs engine" part. How do I request a sync? Should I make another threshold value for triggering the sync, or should I just do it at the same time as persistAllTabs, since that tab store will only be used for sync anyway?

Also, I made my SCORE_THRESHOLD value low for testing. I'm not sure what a real value should be.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 647763 [details] [diff] [review]
WIP

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

::: mobile/android/base/Tabs.java
@@ +29,5 @@
>      private ContentResolver mResolver;
>      private boolean mRestoringSession;
>  
> +    // Keeps track of how much has happened since we last updated our persistent tab store
> +    private int mScore;

Make this `volatile`. If you're worried about increments and flushing happening at the same time, then make it an Atomic*, or go a step further and synchronize access to it. (Making onTabChanged synchronized would probably be enough.)

Note that you're also worried about mOrder, so a lock on all public methods will do the trick (albeit as a big hammer).

@@ +32,5 @@
> +    // Keeps track of how much has happened since we last updated our persistent tab store
> +    private int mScore;
> +
> +    private static final int SCORE_INCREMENT_SMALL = 1;
> +    private static final int SCORE_INCREMENT_MEDIUM = 10;

I'd be inclined to call these SCORE_INCREMENT_TAB_LOCATION_CHANGE and SCORE_INCREMENT_TAB_CLOSED. And I might go so far as to make SMALL ~5; right now you need to navigate to thirty pages before we flush to disk.

@@ +378,5 @@
> +                mScore += SCORE_INCREMENT_SMALL;
> +                break;
> +
> +            // We always get a SELECTED event when adding a new tab, so listening
> +            // for ADDDED would be redundant.

s/ADDDED/ADDED.

@@ +380,5 @@
> +
> +            // We always get a SELECTED event when adding a new tab, so listening
> +            // for ADDDED would be redundant.
> +            case SELECTED:
> +                tab.setLastUsed(System.currentTimeMillis());

Three points here.

1. What's wrong with tab.onChange(), which would bump its own time? IMO telling the tab "you were last used at 1234" is backward OO.

2. A tab is last used when you switch *away* from it, right? Any tab change event should really bump *two* tabs.

3. The same goes for closing a tab. Presumably the new tab gets SELECTED?

@@ +401,5 @@
> +
> +                // Blow away all local tabs and replace them with the current set of tabs.
> +                TabsAccessor.deleteLocalTabs(mResolver);
> +                for (int i = 0; i < mOrder.size(); i++) {
> +                    TabsAccessor.insertLocalTab(mResolver, i, mOrder.get(i));

We should switch TabsAccessor to do batch inserts. It should make a big difference in number of IO ops and resolver interactions.

::: mobile/android/base/TabsAccessor.java
@@ +170,5 @@
> +        cr.delete(BrowserContract.Tabs.CONTENT_URI, LOCAL_TABS_SELECTION, null);
> +    }
> +
> +    // Inserts a tab with a null guid, which corresponds to the local client.
> +    public static long insertLocalTab(final ContentResolver cr, int position, Tab tab) {

Batch insert. Take an array of tabs and an offset, perhaps.

Comment 21

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #19)
> I just realized I forgot to include the "request a sync for the tabs engine"
> part. How do I request a sync? Should I make another threshold value for
> triggering the sync, or should I just do it at the same time as
> persistAllTabs, since that tab store will only be used for sync anyway?

I think you're fine with doing a soft sync request after persisting. Android will throttle as needed.

See pastes in IRC:

17:02:53 < rnewman> margaret: here's the tightly coupled way to do it: https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/setup/activities/SendTabActivity.java#L153
17:03:20 < rnewman> note that it's tightly bound to the account, the SyncAdapter class, and the name of the stage
17:06:08 < rnewman> this might help: http://www.skoumal.net/en/android-run-your-syncadapter-immediately

Comment 22

[:Margaret Leibovic]

Attachment: patch

I tested this with a throwaway sync account, and I was hoping that this would magically make my fennec tabs appear in about:sync-tabs, but it doesn't. Is there something that needs hooking up on the sync side, or am I requesting this sync improperly?

Comment 23

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #22)
> Is there something that needs hooking up on the sync side, or am I
> requesting this sync improperly?

Nope, we need to build that part :D

Shouldn't take too long, so I might do it in the course of reviewing/testing this code.

Thanks!

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 648501 [details] [diff] [review]
patch

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

r- for a couple of concurrency issues, and consideration for bulkInsert. One more quick go-around!

::: mobile/android/base/Tab.java
@@ +118,5 @@
> +        mLastUsed = System.currentTimeMillis();
> +    }
> +
> +    public long getLastUsed() {
> +        return mLastUsed;

This is a `long`, so strictly speaking these two methods should be `synchronized` to avoid garbage return values.

@@ +131,5 @@
>          return mUrl;
>      }
>  
> +    public String getTitle() {
> +        return mTitle;

Similar concurrency issue.

::: mobile/android/base/Tabs.java
@@ +382,5 @@
> +            case LOCATION_CHANGE:
> +                mScore += SCORE_INCREMENT_TAB_LOCATION_CHANGE;
> +                break;
> +
> +            // When one tab is unselected, another one is always selcted, so only

Typo. (And "deselected"?)

@@ +389,5 @@
> +            // for ADDED/CLOSED events.
> +            case SELECTED:
> +                mScore += SCORE_INCREMENT_TAB_SELECTED;
> +            case UNSELECTED:
> +                tab.onChange();                

Trailing whitespace.

@@ +403,5 @@
> +    // This method flushes the current set of tabs to our tabs content provider.
> +    public void persistAllTabs() {
> +        GeckoAppShell.getHandler().post(new Runnable() {
> +            public void run() {
> +                Log.w(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - start of persistAllTabs runnable");

s/w/v

@@ +406,5 @@
> +            public void run() {
> +                Log.w(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - start of persistAllTabs runnable");
> +                // Blow away all local tabs and replace them with the current set of tabs.
> +                TabsAccessor.deleteLocalTabs(mResolver);
> +                TabsAccessor.insertLocalTabs(mResolver, mOrder);

You're accessing `mOrder` here outside of a `synchronized` block.

You probably want a single synchronized method on `TabsAccessor` that does the right thing, or a synchronized accessor that gives you an immutable copy of the array.

(Note that I would encourage you to avoid the use of `setResolver` if you can, in favor of a constructor argument, or at least annotate `mResolver` as "pseudo-final" -- it's theoretically possible for `mResolver` to change between these two calls, which would lead to inconsistency.)

@@ +410,5 @@
> +                TabsAccessor.insertLocalTabs(mResolver, mOrder);
> +                TabsAccessor.updateLocalClient(mResolver);
> +
> +                // Look for a sync account, and trigger a sync if one exists.
> +                Account[] accts = AccountManager.get(mActivity).getAccountsByType(GlobalConstants.ACCOUNTTYPE_SYNC);

Use `SyncAccounts.syncAccountsExist()`.

I would probably suggest splitting this bit of functionality -- prompting a sync -- out into a follow-up. Easier to back out if we need to, and it means I can land a method like `SyncAccounts.requestSync()` alongside it.

@@ +416,5 @@
> +                    Bundle extras = new Bundle();
> +                    extras.putBoolean(ContentResolver.SYNC_EXTRAS_MANUAL, true);
> +                    ContentResolver.requestSync(accts[0], BrowserContract.TABS_AUTHORITY, extras);
> +                }
> +                Log.w(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - end of persistAllTabs runnable");

s/w/v

::: mobile/android/base/TabsAccessor.java
@@ +185,5 @@
> +
> +            // We don't have access to session history in Java, so for now, we'll
> +            // just use a JSONArray that holds most recent history item.
> +            JSONArray history = new JSONArray();
> +            history.put(tab.getURL());

Reuse `history`; use the `put(index, item)` variant. You're only using it for string serialization.

@@ +192,5 @@
> +            values.put(BrowserContract.Tabs.POSITION, i);
> +
> +            valuesToInsert[i] = values;
> +        }
> +        cr.bulkInsert(BrowserContract.Tabs.CONTENT_URI, valuesToInsert);

Remember, this can throw. Should `insertLocalTabs` be best-effort? Is it called within a handler?

::: mobile/android/base/db/TabsProvider.java.in
@@ +158,5 @@
> +
> +            createLocalClient(db);
> +        }
> +
> +        // Insert a client row for our local fennec client

Nit: caps and punctuation.

@@ +160,5 @@
> +        }
> +
> +        // Insert a client row for our local fennec client
> +        private void createLocalClient(SQLiteDatabase db) {
> +            debug("Inserting local fennec client into " + TABLE_CLIENTS + " table");

Ditto.

@@ +632,5 @@
> +        try {
> +            for (int i = 0; i < numValues; i++) {
> +                insertInTransaction(uri, values[i]);
> +                successes++;
> +            }

This approach will cause bulkInsert to throw unless successes == values.length.

You might instead intend to use the "catch and start a new transaction" idiom, which will allow you to continue with subsequent insertions.

Comment 25

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #24)

Thanks for your help. I'm a thread-safe programming n00b :)

> ::: mobile/android/base/Tab.java
> @@ +118,5 @@
> > +        mLastUsed = System.currentTimeMillis();
> > +    }
> > +
> > +    public long getLastUsed() {
> > +        return mLastUsed;
> 
> This is a `long`, so strictly speaking these two methods should be
> `synchronized` to avoid garbage return values.
> 
> @@ +131,5 @@
> >          return mUrl;
> >      }
> >  
> > +    public String getTitle() {
> > +        return mTitle;
> 
> Similar concurrency issue.

Won't I run into this issue with all the existing getters I'm using as well? Should I make them all synchronized?

> ::: mobile/android/base/Tabs.java
> @@ +382,5 @@
> > +            case LOCATION_CHANGE:
> > +                mScore += SCORE_INCREMENT_TAB_LOCATION_CHANGE;
> > +                break;
> > +
> > +            // When one tab is unselected, another one is always selcted, so only
> 
> Typo. (And "deselected"?)

We use UNSELECTED as the event name, which actually now that I think about it, is grammatically incorrect (the event represents a change, not the state of the tab). Sigh, I can update the comment :)

Comment 26

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #25)

> Won't I run into this issue with all the existing getters I'm using as well?
> Should I make them all synchronized?

Consider these example comments :D

Any members that could be accessed from more than one thread -- whether directly or through getters and setters -- need to be synchronized or volatile in unison.

The "in unison" is the important part: it's not enough to make `setTitle` synchronized, if two callers both do

  tab.setURL("http://...");
  tab.setTitle("foo");
  tab.onChanged();

-- the operations can interleave, leaving you with "thread-safe" nonsense: a tab with A's title but B's URL.

(Contrived example, but bear with me.)

If you're relying on synchronized methods for safety, you need to make sure that the class exposes very coarse methods, such that callers don't need to invoke multiple methods sequentially to achieve correct results.

The common alternative is perhaps worse: have the _callers_ synchronize appropriately:

  synchronized (tab) {
    tab.setURL(...);
    ...
  }

This is obviously unpleasant and error prone.

This is why I encouraged the use of executors or similar event queuing operations: no (or less) need to synchronize.

Another approach (which I personally prefer) is to use immutability to achieve the same goal; synchronize Tabs, which owns modifying the collection of tabs, and make Tab immutable. When you change a Tab, replace the reference (safely).

If a Tab is just a 'struct' of title, time, URL, etc.., make a new one every time something changes. Not much allocation -- most of those can be shared references. This could be something like:

  Tabs.updateCurrentTab(partial);

which would be

  public synchronized void updateCurrentTab(Tab tab) {
    // We're synchronized! Grab the current tab,
    // steal some of its old values, and swap
    // the new one in its place.
  }

or some variant of this scheme that keeps control in Tabs. But making Tab immutable allows you to freely shuttle them around the application with no fear of other code changing them.

Give it some thought, and/or read JCIP!

Comment 27

[:Margaret Leibovic]

Hrm, I feel like we're opening a can of worms here. Looking at the way we currently update Tab objects, our code was not at all designed to guarantee that each tab will always contain consistent data. Take, for example, our location change handler:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#676

However, in other places, I don't think it actually matters that calls to update the tab data can be interleaved, such as when we're processing DOMTitleChanged events to update the tab's title. This ordering of Gecko events should be preserved, so if we got a DOMTitleChanged event before a location change event, we shouldn't end up with a tab with the wrong title, although I guess this assumes that those messages are handled on the same thread...

I think it's out of the scope of this bug to audit all of the ways we modify tab data, but it sounds like we should file a different bug to do that. I guess this wasn't really a problem before, but if we're taking a snapshot of the tab set at any point in time, we should make an effort to ensure their data isn't nonsense.

Comment 28

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #24)

> @@ +632,5 @@
> > +        try {
> > +            for (int i = 0; i < numValues; i++) {
> > +                insertInTransaction(uri, values[i]);
> > +                successes++;
> > +            }
> 
> This approach will cause bulkInsert to throw unless successes ==
> values.length.
> 
> You might instead intend to use the "catch and start a new transaction"
> idiom, which will allow you to continue with subsequent insertions.

This is copied over from BrowserProvider. Should we also file a bug about fixing this there?

Comment 29

[:Margaret Leibovic]

Attachment: patch v2

Addressed review comments. To address the concurrency issues with the fields on Tab, I synchronized the methods that modify the fields that we care about persisting in the DB. If we think that there are more potential concurrency bugs lurking in our tab code, I think we should investigate in a follow-up.

I decided to use the "catch and start a new transaction" idiom in bulkInsert as suggested, so we shouldn't need to worry about that throwing inside persistLocalTabs.

Comment 30

[:Margaret Leibovic]

Oh, forgot to mention, getContentResolver only has one consumer in AwesomeBar.java, and we're already keeping track of the activity, so I got rid of the call to setContentResolver.

Comment 31

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #27)

> However, in other places, I don't think it actually matters that calls to
> update the tab data can be interleaved, such as when we're processing
> DOMTitleChanged events to update the tab's title. This ordering of Gecko
> events should be preserved, so if we got a DOMTitleChanged event before a
> location change event, we shouldn't end up with a tab with the wrong title,
> although I guess this assumes that those messages are handled on the same
> thread...

There are probably plenty of those assumptions scattered around: that Java stuff will only be called from Gecko event handlers. And in those situations, concurrency isn't much of a worry.

(I would love for us to reach a point of annotating @NotThreadSafe etc. around the codebase, if only because it ensures that people have thought about this!)

Fixing those assumptions might require a moderate amount of real restructuring: higher-level synchronization, executor queues, or immutability (or all three).

> I think it's out of the scope of this bug to audit all of the ways we modify
> tab data, but it sounds like we should file a different bug to do that. I
> guess this wasn't really a problem before, but if we're taking a snapshot of
> the tab set at any point in time, we should make an effort to ensure their
> data isn't nonsense.

Agreed.

(In reply to Margaret Leibovic [:margaret] from comment #30)
> Oh, forgot to mention, getContentResolver only has one consumer in
> AwesomeBar.java, and we're already keeping track of the activity, so I got
> rid of the call to setContentResolver.

Hurrah!

(In reply to Margaret Leibovic [:margaret] from comment #28)
> This is copied over from BrowserProvider. Should we also file a bug about
> fixing this there?

We should really fix the copypasta, rather than schlepping fixes around between the copies. If we have a bunch of shared functionality between our providers, either inherit or compose to share the logic. But again: follow-up.

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 648868 [details] [diff] [review]
patch v2

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

Summary:

* You might be stuffing nulls into the DB.
* We can safely iterate over a snapshot of mOrder.
* Careful with logging (existing code, file follow-up).
* Remove debug logging :D
* Minor other observations.

::: mobile/android/base/Tab.java
@@ +71,5 @@
>      public static final int STATE_LOADING = 1;
>      public static final int STATE_SUCCESS = 2;
>      public static final int STATE_ERROR = 3;
>  
>      public Tab(int id, String url, boolean external, int parentId, String title) {

Is `url` allowed to be null? Or `title`? You're not using setTitle(title) here, so the signature allows it.

@@ +246,3 @@
>          if (url != null && url.length() > 0) {
>              mUrl = url;
>              Log.i(LOGTAG, "Updated url: " + url + " for tab with id: " + mId);

While we're here, this should really be debug, or even gone altogether: elsewhere we've regarded writing of a user's web activity (including URLs) to the log as personally identifiable information.

@@ +271,3 @@
>          mTitle = (title == null ? "" : title);
>  
>          Log.i(LOGTAG, "Updated title: " + mTitle + " for tab with id: " + mId);

Same.

@@ +359,5 @@
>          // We use -1 to represent icons with sizes="any".
>          if (size == -1 || size >= mFaviconSize) {
>              mFaviconUrl = faviconUrl;
>              mFaviconSize = size;
>              Log.i(LOGTAG, "Updated favicon URL for tab with id: " + mId);

Debug.

::: mobile/android/base/Tabs.java
@@ +9,2 @@
>  import org.mozilla.gecko.db.BrowserDB;
> +import org.mozilla.gecko.sync.GlobalConstants;

Nit: unused imports.

@@ +26,5 @@
>      private static final String LOGTAG = "GeckoTabs";
>  
>      private Tab mSelectedTab;
>      private HashMap<Integer, Tab> mTabs;
>      private ArrayList<Tab> mOrder;

Can we make this `final`? In fact, might as well assign it here, too, along with mScore and mTabs.

Yay for `final`!

@@ +40,1 @@
>      private GeckoApp mActivity;

Are we expecting this to ever change? If not, comment that it's pseudo-final (or, better, make it final and add a constructor argument!).

If it is going to change, then that means it can change half-way through the call inside persistAllTabs(). I'd love for us to move towards eliminating this, and having it as an argument to methods that need it.

No need to go sprinkling more synchronization right now, but be aware that there's a bug here.

@@ +222,5 @@
>      public ArrayList<Tab> getTabsInOrder() {
>          if (getCount() == 0)
>              return null;
>  
>          return mOrder;

I think we want to make a small change here.

There are two places getTabsInOrder() gets called -- TabsTray.TabsAdapter.refreshTabsData(), and GeckoApp.loadUrlInTab(url). In both cases, we simply want to iterate over the resultant array.

Furthermore, we definitely don't want it mutating underneath us! If mOrder changes during the iteration in either of those methods, we'll get a ConcurrentModificationException at best, or undefined behavior at worst.

And down in persistAllTabs, you access mOrder directly.

I suggest we have getTabsInOrder return Iterable<Tab>, not ArrayList<Tab>, alter the call sites accordingly, change the type of mOrder to be CopyOnWriteArrayList<Tab>, and fix persistAllTabs() to match.

That will ensure that whenever a caller fetches the tab list, it gets a stable read-only snapshot. Handling modification comes for free with COWAL, albeit at the cost of allocation instead of synchronization.

You could also do a kind of "CopyOnRead...", by making a stable copy of the tabs right here in getTabsInOrder, but that just introduces allocation in the read case.

@@ +395,5 @@
> +    // This method persists the current ordered list of tabs in our tabs content provider.
> +    public void persistAllTabs() {
> +        GeckoAppShell.getHandler().post(new Runnable() {
> +            public void run() {
> +                TabsAccessor.persistLocalTabs(mActivity.getContentResolver(), mOrder);

getTabsInOrder().

::: mobile/android/base/TabsAccessor.java
@@ +174,5 @@
> +    // Inserts tabs with a null guid, which corresponds to the local client.
> +    // The tabs param should be an ordered list of tabs that reflects how they should
> +    // be positioned in the DB.
> +    private static void insertLocalTabs(final ContentResolver cr, ArrayList<Tab> tabs) {
> +        // Used for serializing a history array.

A little bit "add 1 to x". Perhaps

// Reuse this for serializing individual history URLs as JSON.

@@ +185,5 @@
> +            ContentValues values = new ContentValues();
> +            values.put(BrowserContract.Tabs.TITLE, tab.getTitle());
> +            values.put(BrowserContract.Tabs.URL, tab.getURL());
> +            values.put(BrowserContract.Tabs.FAVICON, tab.getFaviconURL());
> +            values.put(BrowserContract.Tabs.LAST_USED, tab.getLastUsed());

If any of these values are null, you have to use putNull(entry) instead.

::: mobile/android/base/db/TabsProvider.java.in
@@ +121,5 @@
>          }
>  
>          @Override
>          public void onCreate(SQLiteDatabase db) {
> +            Log.i("BOOM", "TabsProvider onCreate");

Heh.

Comment 33

[Richard Newman [:rnewman]]

Margaret, your v2 patch fails for me on current m-i. Looks like someone else (or you!) has been touching Tab/Tabs/TabsAccessor. 

Could you un-bitrot plz?

(Also switching to block, not depend on, Bug 780279, because I think that's more accurate.)

Comment 34

[:Margaret Leibovic]

Attachment: patch (v2 - unbitrotted)

I just landed some changes to Tab.java on inbound, so you may need to pull again before applying this!

Comment 35

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #32)

> ::: mobile/android/base/Tab.java
> @@ +71,5 @@
> >      public static final int STATE_LOADING = 1;
> >      public static final int STATE_SUCCESS = 2;
> >      public static final int STATE_ERROR = 3;
> >  
> >      public Tab(int id, String url, boolean external, int parentId, String title) {
> 
> Is `url` allowed to be null? Or `title`? You're not using setTitle(title)
> here, so the signature allows it.

`url` can be null, as mentioned in a comment down below, so I'm now guarding against that in TabsAccessor. And I updated our mTitle assignment to ensure that it's never null.


> @@ +40,1 @@
> >      private GeckoApp mActivity;
> 
> Are we expecting this to ever change? If not, comment that it's pseudo-final
> (or, better, make it final and add a constructor argument!).
> 
> If it is going to change, then that means it can change half-way through the
> call inside persistAllTabs(). I'd love for us to move towards eliminating
> this, and having it as an argument to methods that need it.

I don't think it should change for the life of the app, but it's not a constructor argument because Tabs is implemented as a singleton. Holding onto mActivity a new thing, and it's an attempt at avoiding references to GeckoApp.mAppContext. Sriram has been working on this, and I think it's something we should address in a separate bug.

New patch coming soon...

Comment 36

[:Margaret Leibovic]

Attachment: patch v3

Comment 37

[Richard Newman [:rnewman]]

Comment on attachment 649435 [details] [diff] [review]
patch v3

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

Good work!

::: mobile/android/base/GeckoApp.java
@@ +2525,5 @@
>      public void loadUrlInTab(String url) {
> +        Iterable<Tab> tabs = Tabs.getInstance().getTabsInOrder();
> +        for (Tab tab : tabs) {
> +            if (url.equals(tab.getURL())) {
> +                Tabs.getInstance().selectTab(tab.getId());

Nit: call Tabs.getInstance() once in this entire method.

::: mobile/android/base/Tab.java
@@ +33,5 @@
>  public final class Tab {
>      private static final String LOGTAG = "GeckoTab";
>  
>      private static Pattern sColorPattern;
>      private int mId;

Nit: mId can (and should? check that tests don't rely on it being mutable) be final.

@@ +245,3 @@
>          if (url != null && url.length() > 0) {
>              mUrl = url;
> +            Log.d(LOGTAG, "Updated url for tab with id: " + mId);

Nit: I prefer "URL".

::: mobile/android/base/Tabs.java
@@ +30,3 @@
>      private boolean mRestoringSession;
>  
> +    // Keeps track of how much has happened since we last updated our persistent tab store

Nit: closing period.

@@ +380,5 @@
> +
> +    // This method persists the current ordered list of tabs in our tabs content provider.
> +    public void persistAllTabs() {
> +        GeckoAppShell.getHandler().post(new Runnable() {
> +            public void run() {

@Override.

@@ +381,5 @@
> +    // This method persists the current ordered list of tabs in our tabs content provider.
> +    public void persistAllTabs() {
> +        GeckoAppShell.getHandler().post(new Runnable() {
> +            public void run() {
> +                TabsAccessor.persistLocalTabs(mActivity.getContentResolver(), getTabsInOrder());

Use our own getContentResolver() method here, rather than poking at mActivity.

Also, minor order of evaluation tweak: do you want to snapshot the tabs now, and persist them in the future, or persist the tabs at the time that this Runnable is executed? That dictates whether you want to lift the call to getTabsInOrder() out of the Runnable:

  final Iterable<Tab> tabs = getTabsInOrder();
  GeckoAppShell.getHandler().… {
    …
  }

I imagine you'd want to do this if someone called:

  Tabs.persistAllTabs();
  clearAllOpenTabsGivenThatWeJustPersistedThem();

because the Gecko background thread might not run your Runnable before the cleanup happens!

::: mobile/android/base/TabsAccessor.java
@@ +192,5 @@
> +            // Skip this tab if it has a null URL.
> +            String url = tab.getURL();
> +            if (url == null)
> +                continue;
> +            

Whitespace.

@@ +221,5 @@
> +
> +            valuesToInsert.add(values);
> +        }
> +
> +        ContentValues[] valuesToInsertArray = valuesToInsert.toArray(new ContentValues[valuesToInsert.size()]);

Y'know, you could have bulkInsert skip over nulls, and just stuff your ContentValues straight into a ContentValues[tabs.size()]… no big deal if you can't be bothered :D

::: mobile/android/base/TabsTray.java
@@ +195,5 @@
>  
>          private void refreshTabsData() {
>              // Store a different copy of the tabs, so that we don't have to worry about
>              // accidentally updating it on the wrong thread.
>              mTabs = new ArrayList<Tab>();

Optional: if you do this allocation after the call to `tabs`, you could ask Tabs for a count, and potentially avoid a reallocation inside ArrayList.

Comment 38

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #37)

> @@ +380,5 @@
> > +
> > +    // This method persists the current ordered list of tabs in our tabs content provider.
> > +    public void persistAllTabs() {
> > +        GeckoAppShell.getHandler().post(new Runnable() {
> > +            public void run() {
> 
> @Override.

Why? We don't do that elsewhere in our code, so if that's a problem, I can file a bug to fix that.

Comment 39

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #38)

> > @Override.
> 
> Why? We don't do that elsewhere in our code, so if that's a problem, I can
> file a bug to fix that.

Three reasons leap to mind:

* If an upstream method changes signature, you'll get a compilation error rather than a silent change in which method is called. Oh, and if you typo, or omit an argument, the compiler will catch it.

* It tells readers of your intent: this is not an accidental method collision.

* It'll help anyone using an IDE: it will avoid yellow flags, and it allows the IDE to help with refactorings.

In short: it's really, really cheap insurance and documentation, and Java best practice.

No need to spend hours walking the whole codebase to add them, but fix when you're touching code (you might find bugs!), and ensure the right annotations are present when you're adding new code.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

(In reply to Richard Newman [:rnewman] from comment #39)
> Three reasons leap to mind:

Drive-by comment: For methods that override an existing method with a body, I totally agree that we should use @Override. For methods that implement an interface's method declaration (or an abstract method delcaration) I don't think it's necessary. In fact, doing so caused compilation errors in some javac versions.

Do IDEs give yellow flags if you skip the @Override on a Runnable.run implementation?

Comment 41

[Richard Newman [:rnewman]]

(In reply to Kartikaya Gupta (:kats) from comment #40)
> (In reply to Richard Newman [:rnewman] from comment #39)
> > Three reasons leap to mind:
> 
> Drive-by comment: For methods that override an existing method with a body,
> I totally agree that we should use @Override. For methods that implement an
> interface's method declaration (or an abstract method delcaration) I don't
> think it's necessary. In fact, doing so caused compilation errors in some
> javac versions.

Interesting!

All of Android Sync's code uses @Override on interface methods, including Runnable.run. Is this just <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5008260> ?

(Do we support Java 1.5?)

I agree that most of the benefit comes when overriding a superclass implementation. But I find @Override kinda useful even for interface implementations, because interfaces can change just like superclasses. Granted, for interfaces you get a compile error if a signature changes such that you now don't implement the whole interface, but that benefit goes away if you're implementing an abstract class… of which there are 30-odd in Android Sync alone.

I don't like having to think any more than necessary, so I like to trust that if it doesn't say @Override, it's a new method, and I don't have to add or remove annotations if I refactor a class to make it concrete or abstract, etc.

> Do IDEs give yellow flags if you skip the @Override on a Runnable.run
> implementation?

Eclipse by default doesn't turn on most of its errors and warnings. "Missing @Override annotation" is in there, off by default, with a checkbox for "Include implementations of interface methods (1.6 and higher)", which is on by default if the annotation check is included.

Comment 42

[Kartikaya Gupta (email:[e-mail])]

(In reply to Richard Newman [:rnewman] from comment #41)
> All of Android Sync's code uses @Override on interface methods, including
> Runnable.run. Is this just
> <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5008260> ?
> 

Yup. The change to allow @Override on interface methods was added in java 6, and I think backported to some later versions of 1.5 as well.

> (Do we support Java 1.5?)
> 

I thought we did way back when, but probably don't anymore.

> I agree that most of the benefit comes when overriding a superclass
> implementation. But I find @Override kinda useful even for interface
> implementations, because interfaces can change just like superclasses.
> Granted, for interfaces you get a compile error if a signature changes such
> that you now don't implement the whole interface, but that benefit goes away
> if you're implementing an abstract class… of which there are 30-odd in
> Android Sync alone.
> 
> I don't like having to think any more than necessary, so I like to trust
> that if it doesn't say @Override, it's a new method, and I don't have to add
> or remove annotations if I refactor a class to make it concrete or abstract,
> etc.

Yeah, there's some interesting points on the bug you linked above as well, with abstract classes accidentally mangling the signature of a method they inherit from an interface and it not getting caught. It does sound like @Override on interfaces and abstract methods would catch some classes of errors, so I retract my previous concerns.

Comment 43

[:Margaret Leibovic]

Before seeing that last comment, I had decided to omit the @Override because that's consistent with what we currently do in the fennec codebase, but I can land a follow-up if we think it's worthwhile to add the @Override.

https://hg.mozilla.org/integration/mozilla-inbound/rev/afc4105801f9

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afc4105801f9

Comment 45

[Richard Newman [:rnewman]]

(In reply to Margaret Leibovic [:margaret] from comment #43)
> Before seeing that last comment, I had decided to omit the @Override because
> that's consistent with what we currently do in the fennec codebase, but I
> can land a follow-up if we think it's worthwhile to add the @Override.

Not a big deal; just nudging course for the future!

Comment 46

[Nick Alexander :nalexander [he/him]]

This is all kinds of awesome, but in the course of implementing Bug 780279 and Bug 728612 I've found the scoring delay to be counter-intuitive.  What often happens is that you open one new tab and then force a sync, which then doesn't actually sync the new tab since it hasn't been persisted to disk yet.

How would we feel about following on with an onIdle() handler that persists tabs to disk if the score is positive?  We could ensure that Bug 780279 doesn't start syncing too frequently.

If onIdle() is too aggressive, could we try to persist to disk (if the score is positive) for each query(), perhaps only if Fennec is already running?

Comment 47

[Nick Alexander :nalexander [he/him]]

Follow up: my guess is this is responsible for a possible bug: the open tabs in Fennec and the Synced Tabs shown for the device can be different.  Not cool.  We should probably filter the current device from the Fennec Synced Tabs list.

(No bug filed yet, since the feature is still in development.)

Comment 48

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #46)
> This is all kinds of awesome, but in the course of implementing Bug 780279
> and Bug 728612 I've found the scoring delay to be counter-intuitive.  What
> often happens is that you open one new tab and then force a sync, which then
> doesn't actually sync the new tab since it hasn't been persisted to disk yet.
> 
> How would we feel about following on with an onIdle() handler that persists
> tabs to disk if the score is positive?  We could ensure that Bug 780279
> doesn't start syncing too frequently.
> 
> If onIdle() is too aggressive, could we try to persist to disk (if the score
> is positive) for each query(), perhaps only if Fennec is already running?

I might be misunderstanding what you wrote, but surely it's enough to simply request a sync after persisting, and not otherwise?

There's never any point in syncing if there's no fresh data. 

(True regardless of whether persisting happens on idle or in some other mechanism; these are two separate concerns.)

Comment 49

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #48)
> (In reply to Nick Alexander :nalexander from comment #46)
> > This is all kinds of awesome, but in the course of implementing Bug 780279
> > and Bug 728612 I've found the scoring delay to be counter-intuitive.  What
> > often happens is that you open one new tab and then force a sync, which then
> > doesn't actually sync the new tab since it hasn't been persisted to disk yet.
> > 
> > How would we feel about following on with an onIdle() handler that persists
> > tabs to disk if the score is positive?  We could ensure that Bug 780279
> > doesn't start syncing too frequently.
> > 
> > If onIdle() is too aggressive, could we try to persist to disk (if the score
> > is positive) for each query(), perhaps only if Fennec is already running?
> 
> I might be misunderstanding what you wrote, but surely it's enough to simply
> request a sync after persisting, and not otherwise?


I agree that requesting a sync after persisting is good; that's Bug 780279.

The issue is that the disk state and Fennec's state don't actually agree very often.  If you open (say) N tabs one after the other, then after K < N of them the scoring algorithm persists to disk.  We then request a sync.  But the remaining N - K are not persisted until some other tab activity happens, which can be indefinitely long.  The result is that your synced tabs are almost never correct!

What I think we should add is a periodic persist (and subsequent sync) so that the disk state and the Fennec state agree more of the time.