Extension Settings API: throttle the API in the same way that the Bookmarks API

Extension Settings API: throttle the API in the same way that the Bookmarks API
is throttled.


BUG=98498
TEST=*ExtensionSettings*


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

[not at google - send to devlin, 2011-11-11 03:06:01]

Note: I just basically copied what the Bookmarks API does, with a few
modifications from what I understand of how it works (code has been commented).

Tim/Fred, does this look sane?

[Matt Perry, 2011-11-11 03:23:07]

Is this really what we want to do? What if it worked the following way:
- don't ever throttle the extension
- once we hit the sync limit, updates to settings stay local, but are queued up
and synced on a schedule that sync can handle

Is that possible/desirable?

[not at google - send to devlin, 2011-11-11 03:39:26]

On 2011/11/11 03:23:07, Matt Perry wrote:
> Is this really what we want to do? What if it worked the following way:
> - don't ever throttle the extension
> - once we hit the sync limit, updates to settings stay local, but are queued
up
> and synced on a schedule that sync can handle
> 
> Is that possible/desirable?

I think it's desirable (since it's purely a sync concern), but
difficult/complicated to get right.  Nothing is impossible, just... impractical.

The main issue I can think of is that if sync changes are buffered but not sent,
what happens if a change is received from sync?  Usually the sync infrastructure
handles things like this, but now that we're handling this ourselves
per-extension it gets tough.

I guess you could... have the buffer be a map of key->change, then when a change
is received from sync, apply it locally and remove that key from the map.  Then,
when sync is ready to send again, send all the changes at once.

In general though, it becomes harder to reason about the state of sync w.r.t.
the state of local settings when you start doing things like that (which is why
I switched over to using an non-synced always-failing storage, rather than a
synced in-memory object, if leveldb creation fails).

In any case.. . somebody from the sync team could probably comment more
informedly about what decision led to not just buffering sync changes for the
bookmarks API.

[not at google - send to devlin, 2011-11-14 07:51:28]

I might go ahead and give this a go, actually.  The general algorithm would be
like, pseudo-C++:

***

(syncable_settings_storage.h)

class SyncableSettingsStorage {
  ...

  // (Pull out whatever dependencies needed here.)
  ExtensionQuotaService quota_service_;

  // Changes waiting to go out to sync when there is quota.
  std::map<std::string, SyncChange> pending_outgoing_;

  ...
}


(syncable_settings_storage.cc)

SyncableSettingsStorage::SendChangesToSync(const SettingChangeList& changes) {
  SyncChangeList changes;

  ... (build changes)

  if (quota_service_.HasQuota()) {
    // Add previously buffered changes to changes_, probably from start.
    for (std::map<std::string, SyncChange>::iterator i =
pending_outgoing_.begin();
        i != pending_outgoing_.end(); ++i) {
      changes.push_back(i->second);
    }
    pending_outgoing_.clear();
    syncable_service_->Send(changes);
  } else {
    // Buffer the changes.
    for (SyncChangeList::iterator i = changes.begin();
        i != changes.end(); ++i) {
      pending_outgoing_[GetTag(*i)] = *i;
    }
  }
}

SyncableSettingsStorage::ProcessSyncChanges(SyncChangeList changes) {
  // Invalidate any pending changes for updated keys.
  for (SyncChangeList::iterator i = changes.begin();
      i != changes.end(); ++i) {
    pending_outgoing_.erase(GetTag(*i));
  }

  ...
}

***

The only thing is, I'm not sure how easy it will be to pull out
ExtensionQuotaService into a class like that.  It would be preferable to do that
rather than writing a simple quota system from scratch (even though, as you can
see from the code I've already written here, the restrictions for settings are
less complex than those for bookmarks).

Or just to go with the bookmarks way because it's much less code.

Either way I'll put it off for another couple of days.

[not at google - send to devlin, 2011-11-14 07:54:42]

(Oops, forgot to do quota_service_.SubtractQuota(changes) or something. 
Whatever).

The main behavioural disadvantage of this is that the outgoing changes map would
only be cleared when a setting changes and goes to sync... it would perhaps be
"nicer" to (re-)schedule some task(s) to clear it later.

That said, the main purpose of this stuff is to discourage badly written
extensions from killing settings sync.

So maybe re-using the bookmarks stuff is easiest after all.

[Matt Perry, 2011-11-14 19:15:49]

On 2011/11/14 07:54:42, kalman wrote:
> (Oops, forgot to do quota_service_.SubtractQuota(changes) or something. 
> Whatever).
> 
> The main behavioural disadvantage of this is that the outgoing changes map
would
> only be cleared when a setting changes and goes to sync... it would perhaps be
> "nicer" to (re-)schedule some task(s) to clear it later.
> 
> That said, the main purpose of this stuff is to discourage badly written
> extensions from killing settings sync.
> 
> So maybe re-using the bookmarks stuff is easiest after all.

There may have been a reason the bookmarks quota was written that way, so I'd
wait to hear back from the sync guys. I certainly believe it's an easier
approach. It would be cool if we could queue up local changes, though.

[tim (not reviewing), 2011-11-14 21:22:09]

The ExtensionsQuotaService was initially written primarily so that poorly
written extensions (and e.g. bugs) wouldn't generate garbage sync traffic. We
saw this (on O(millions)) happening with several bookmark related extensions
before this was put in. It's not designed to be 100% airtight in fully
preventing a developer from circumventing the throttles if he's going out of his
way to game the system.  We keep the big guns server-side to prevent abuse, but
unfortunately these are typically targeted at blocking a user, and not just the
extension. If a particular extension is going out of control committing globally
across many users, we do have mechanisms to block only that extension, but this
is only used in extremely abusive circumstances.

So, sometimes we don't want to buffer changes because the changes are
effectively "accidental" spam, and we don't want to generate unnecessary
traffic. We have some generic (non type specific) buffering for changes in sync
already that does exponential backoff and deals with errors or server-directed
throttling, which assigns per-user quotas and throttles the client and is
capable of what Matt mentions (when sync is less busy globally, try to relieve
pressure). We're working on making these quotas per-datatype this quarter, but
not any more granular than that.

Extensions + settings are unlike any other type (that our server throttling was
designed for) in that all the traffic is generated by third parties, so there is
a need for more in-depth throttling limits.  With the think sync API model, any
additional datatype level logic should be built inherently in the type, where
all the properties are understood (extension ID, for example), instead of
special casing the sync infrastructure further. I think this patch is useful at
a minimum to prevent the bug spam traffic case. I think it also helps
well-intentioned third-parties build something that conforms to a needed sanity
threshhold in a way that doesn't hurt other extensions or datatypes, and is a
lot more test / debugging friendly manner (easy resets).  I know that last bit
is more contentious. We could perhaps add a capability to delegate to extensions
code when building a commit request to filter-out commit entries if they exceed
some limit (which would leave them in a modified-but-uncommitted state in sync,
similar to when we can't reach the server), but that would take some time to
build.  I'm not convinced it's better yet...  At some point we have to force
developers to think about being efficient in the same way that we (chromium
devs) are careful when building sync for a datatype.  Returning a hard error to
the developer if a crazy limit is hit is one way to do that. Happy to meet to
discuss this more.

[not at google - send to devlin, 2011-11-16 12:17:31]

Cool, as discussed, sounds like this patch should go in (particularly IMO
because of the developer error-feedback it would give regarding abuse).

[akalin, 2011-11-17 03:38:26]

I'll let tim handle this review, as he knows the throttling stuff better than I
do.

[tim (not reviewing), 2011-11-17 18:59:52]

http://codereview.chromium.org/8491033/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/8491033/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:85: QuotaLimitHeuristics*
heuristics) const {}
I think the bookmark_extension_api is missing the OVERRIDE macro for this?
you'll have to update this elsewhere.

[not at google - send to devlin, 2011-11-18 03:49:00]

http://codereview.chromium.org/8491033/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/8491033/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:85: QuotaLimitHeuristics*
heuristics) const {}
On 2011/11/17 18:59:52, timsteele wrote:
> I think the bookmark_extension_api is missing the OVERRIDE macro for this?
> you'll have to update this elsewhere.

Done.

[tim (not reviewing), 2011-11-18 07:00:00]

On 2011/11/18 03:49:00, kalman wrote:
>
http://codereview.chromium.org/8491033/diff/8001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_function.h (right):
> 
>
http://codereview.chromium.org/8491033/diff/8001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_function.h:85: QuotaLimitHeuristics*
> heuristics) const {}
> On 2011/11/17 18:59:52, timsteele wrote:
> > I think the bookmark_extension_api is missing the OVERRIDE macro for this?
> > you'll have to update this elsewhere.
> 
> Done.

Still seeing some (src/chrome/browser/extensions/extension_webrequest_api.cc)...
just make sure you get 'em all before landing.

The limits themselves look good at least for now.  We haven't set the server
limits in stone yet, I'll let you know if things end up being wildly different.

LGTM from the sync side

[Aaron Boodman, 2011-11-18 07:52:35]

LGTM extensions

[commit-bot: I haz the power, 2011-11-20 01:30:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/8491033/20001

[commit-bot: I haz the power, 2011-11-20 02:36:25]

Change committed as 110854