Apps&Extensions for Supervised Users: send permission request on outdated re-enables

chrome/browser/extensions/app_sync_bundle.cc

Index: chrome/browser/extensions/app_sync_bundle.cc
diff --git a/chrome/browser/extensions/app_sync_bundle.cc b/chrome/browser/extensions/app_sync_bundle.cc
index c50f2c9aa7361b503735c8954024a6ae07080af5..bfd3e19f2f97a6f90ea5a909579ca58d42b99a54 100644
--- a/chrome/browser/extensions/app_sync_bundle.cc
+++ b/chrome/browser/extensions/app_sync_bundle.cc
@@ -120,6 +120,11 @@ bool AppSyncBundle::IsSyncing() const {
 }
 
 void AppSyncBundle::SyncChangeIfNeeded(const Extension& extension) {
+  // If there is a pending update for this app, apply it first.
+  auto it = pending_sync_data_.find(extension.id());
+  if (it != pending_sync_data_.end())
+    ProcessSyncChange(it->second);

[Marc Treib, 2015/06/22 15:35:18]

Before this CL, the pending_sync_data_ was applied only during
MergeDataAndStartSyncing, i.e. it only handled changes that happened before Sync
was up and running. I think that was a bug that just happened to have no visible
effects (see other comments below), but I might be missing something..?

[not at google - send to devlin, 2015/06/22 21:03:10]

I'm struggling with this. Is SyncChangeIfNeeded the right place to be doing
this? This gets called when we try to *generate* a sync change, right? Why would
that involve pushing out these pending sync changes?

[Marc Treib, 2015/06/23 10:22:25]

My reasoning is this: At some previous point, we decided that we can't apply
this sync change right now (e.g. because the installed extension is outdated),
and instead queued it up as pending.
Now here, we're telling the ExtensionSyncService "this extension has changed",
so this is a good time to try applying the change again.
Maybe "SyncChangeIfNeeded" should be renamed to "NotifyExtensionChanged" or
something like that?

[not at google - send to devlin, 2015/06/24 00:25:10]

That doesn't sound like it follows to me. Perhaps there should be a DCHECK in
here that there is no pending sync change for an Extension, but actually pushing
out that change - that should happen when the conditions that caused it to skip
in the first place are fulfilled.

I.e. when we know that ExtensionSyncService::ProcessExtensionSyncDataHelper will
return true.

The funky thing is that ProcessExtensionSyncDataHelper does a whole bunch of
work before deciding that it can apply the change. Calling back into it like
this (calling ProcessSyncChange again) will cause all that work to happen again,
at which point it again may return false (I don't see why not). That seems ...
wrong. In fact if that comment is to believed "Returns false if the changes were
not completely applied and need to be tried again later" and something *does*
try again later... that is exactly the same problem already.

Basically this is all very hard to follow. ExtensionSyncService and *SyncBundle
jump between each other and I lose track of what the role is supposed to be.
Argh.

[Marc Treib, 2015/06/24 11:50:57]

Well, in the current state, ProcessExtensionSyncDataHelper gives us no clue why
it said "false" before, so "retry whenever something changed" is kind of the
best we can do. But maybe we should change that :)
Yeah, that's kind of messed up: Right now, *nothing ever tries again later* (if
Sync is up already).

Anyway, I noticed that my changes right now wouldn't work reliably: The
MarkPendingAppSynced call below will remove the pending Sync change, whether is
was applied or not. So I guess some more thought is required here...
Indeed.

[not at google - send to devlin, 2015/06/24 20:47:38]

Indeed. It wouldn't be too hard of a transformation though it does mildly
terrify me that what should be an innocuous change... well, who knows.
I followed the path up from GetPendingData() and it ends up in
ExtensionSyncService::MergeDataAndStartSyncing() where those pending changes are
applied, and which is called when sync is set up. Does that count as "trying
again" or is there something else going wrong?

[Marc Treib, 2015/06/29 09:52:49]

I'll take a shot at it. Let's see where I'll end up :)
IIUC MergeDataAndStartSyncing is called only once in the lifetime of the
service, and that's when Sync starts up. So if we queue up any pending changes
after Sync has started up, then those will never be applied.

+
   AppSyncData app_sync_data = extension_sync_service_->GetAppSyncData(
       extension);