[Sync] Don't attempt to sync windowless tabs.

[Sync] Don't attempt to sync windowless tabs.

Previously we would create a tab node before realizing a tab was windowless.
Added a check into IsValidTab to prevent this happening. Also ensure we
give priority to a tab being destroyed before checking if its valid. In addition
several changes were made to make us fail more gracefully.

BUG=93172, 92990
TEST=Sync with sessions + instant search

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98339

[Nicolas Zea, 2011-08-22 23:40:55]

Trying a different approach that makes us stricter about creating tab nodes in
the first place. I was able to use instant without issue with this, please apply
locally and see if you can still repro the problems.

[Yaron, 2011-08-23 00:31:54]

Yep, this works too. It's also cleaner in that we never allocate the
unnecessary/unused sync code.

http://codereview.chromium.org/7706024/diff/1/chrome/browser/sync/glue/sessio...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7706024/diff/1/chrome/browser/sync/glue/sessio...
chrome/browser/sync/glue/session_model_associator.cc:583: NOTREACHED();
Do you still want to leave this in? It seems like we could protect ourselves in
this case. Basically, if it's a corrupt entry that's not a tab or a window, just
ignore it but don't fail association.

[Yaron, 2011-08-23 00:41:43]

On 2011/08/23 00:31:54, Yaron wrote:
> Yep, this works too. It's also cleaner in that we never allocate the
> unnecessary/unused sync code.
> 
>
http://codereview.chromium.org/7706024/diff/1/chrome/browser/sync/glue/sessio...
> File chrome/browser/sync/glue/session_model_associator.cc (right):
> 
>
http://codereview.chromium.org/7706024/diff/1/chrome/browser/sync/glue/sessio...
> chrome/browser/sync/glue/session_model_associator.cc:583: NOTREACHED();
> Do you still want to leave this in? It seems like we could protect ourselves
in
> this case. Basically, if it's a corrupt entry that's not a tab or a window,
just
> ignore it but don't fail association.

Actually, I'm still able to create issues with this patch. If I play with it
enough (shutting down and restarting), I'm still able to get into a situation
where it starts to fail syncing new tabs.

When I try and allocate a new node I see "Could not create new node!" and then
subsequent attempts to get a free node always say "Failed to look up tab node
-1". I believe this is because of the issue on the other thread where we always
call mutable_header(), and we just look for the existence of a header() node to
tell if it's a window or not.

[Nicolas Zea, 2011-08-24 22:47:33]

+Drew for the review. Talked with Yaron and what he saw may have been a separate
issue. This definitely fixes some bugs.

[tim (not reviewing), 2011-08-25 20:25:01]

This LGTM

http://codereview.chromium.org/7706024/diff/4001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7706024/diff/4001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:240: if (sync_id == -1) {
isn't there a constant for this -1 id?
also you seem to use no braces on single line if below

http://codereview.chromium.org/7706024/diff/4001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:241: return;
log something, perhaps?

[Nicolas Zea, 2011-08-25 20:52:01]

Committing once trybots go green

http://codereview.chromium.org/7706024/diff/4001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7706024/diff/4001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:240: if (sync_id == -1) {
On 2011/08/25 20:25:01, timsteele wrote:
> isn't there a constant for this -1 id?
> also you seem to use no braces on single line if below

Done.

http://codereview.chromium.org/7706024/diff/4001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:241: return;
On 2011/08/25 20:25:01, timsteele wrote:
> log something, perhaps?

Logging is done in the GetFreeTabNode call

[commit-bot: I haz the power, 2011-08-25 22:42:03]

Presubmit check for 7706024-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
You might be calling functions intended only for testing from
production code. Please verify that the following usages are OK,
and email joi@chromium.org if you are seeing false positives:
  chrome/browser/sync/glue/session_model_associator.cc:194
    if (waiting_for_change_) QuitLoopForTest(); \
  chrome/browser/sync/glue/session_model_associator.cc:213
    if (waiting_for_change_) QuitLoopForTest();