Atomic UserScript ID generation

Atomic UserScript ID generation

BUG=377978
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291489

[Mark Dittmer, 2014-08-21 01:29:09]

Hey Devlin, the correct operation of two upcoming (much larger) CLs depend on
this change in UserScript ID generation.

[not at google - send to devlin, 2014-08-21 01:39:32]

Devlin has gone home

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.h
(right):

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.h:43: static
int64 GetNextUserScriptID();
GenerateUserScriptID()?

This is the sort of change that needs context, but I'm guessing you'll be
calling this from somewhere other than ContentScriptsHandler. In that case I
think you're better off exposing a public method like this on UserScript, not
here.

[Devlin, 2014-08-21 15:27:48]

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:37:
base::StaticAtomicSequenceNumber g_user_script_id_generator;
This seems... overarchitected.  Any advantage over the old?

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:401: int64
ContentScriptsHandler::GetNextUserScriptID() {
GetNext() returns an int, not an int64.  Probably never a problem (unlikely that
we'll overflow 4b), but something to consider.

[Mark Dittmer, 2014-08-21 19:45:41]

Moved counter to UserScript and changed UserScript ID type to "int" (was int64).
This required fixing (un)pickling code to use the right size as well.

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:37:
base::StaticAtomicSequenceNumber g_user_script_id_generator;
On 2014/08/21 15:27:47, Devlin wrote:
> This seems... overarchitected.  Any advantage over the old?

In early testing of https://codereview.chromium.org/493633003/ and
https://codereview.chromium.org/492133002/ I discovered that UserScripts are
instantiated from different threads. This ensures that we don't have duplicate
IDs.

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:401: int64
ContentScriptsHandler::GetNextUserScriptID() {
On 2014/08/21 15:27:47, Devlin wrote:
> GetNext() returns an int, not an int64.  Probably never a problem (unlikely
that
> we'll overflow 4b), but something to consider.

This is done because UserScript IDs are int64. Should I change that to int to
avoid deceptive overflow bugs?

[Devlin, 2014-08-21 20:10:20]

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:401: int64
ContentScriptsHandler::GetNextUserScriptID() {
On 2014/08/21 19:45:41, Mark Dittmer wrote:
> On 2014/08/21 15:27:47, Devlin wrote:
> > GetNext() returns an int, not an int64.  Probably never a problem (unlikely
> that
> > we'll overflow 4b), but something to consider.
> 
> This is done because UserScript IDs are int64. Should I change that to int to
> avoid deceptive overflow bugs?

Yes.  We should be consistent.

[Mark Dittmer, 2014-08-21 21:12:29]

int64 -> int already done.

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:401: int64
ContentScriptsHandler::GetNextUserScriptID() {
On 2014/08/21 20:10:20, Devlin wrote:
> On 2014/08/21 19:45:41, Mark Dittmer wrote:
> > On 2014/08/21 15:27:47, Devlin wrote:
> > > GetNext() returns an int, not an int64.  Probably never a problem
(unlikely
> > that
> > > we'll overflow 4b), but something to consider.
> > 
> > This is done because UserScript IDs are int64. Should I change that to int
to
> > avoid deceptive overflow bugs?
> 
> Yes.  We should be consistent.

Done.

[Devlin, 2014-08-21 21:24:23]

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/495853002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:401: int64
ContentScriptsHandler::GetNextUserScriptID() {
On 2014/08/21 21:12:29, Mark Dittmer wrote:
> On 2014/08/21 20:10:20, Devlin wrote:
> > On 2014/08/21 19:45:41, Mark Dittmer wrote:
> > > On 2014/08/21 15:27:47, Devlin wrote:
> > > > GetNext() returns an int, not an int64.  Probably never a problem
> (unlikely
> > > that
> > > > we'll overflow 4b), but something to consider.
> > > 
> > > This is done because UserScript IDs are int64. Should I change that to int
> to
> > > avoid deceptive overflow bugs?
> > 
> > Yes.  We should be consistent.
> 
> Done.

You're missing some.  What about all the renderer places?

[Devlin, 2014-08-22 18:24:21]

lgtm

https://codereview.chromium.org/495853002/diff/40001/extensions/common/user_s...
File extensions/common/user_script.cc (right):

https://codereview.chromium.org/495853002/diff/40001/extensions/common/user_s...
extensions/common/user_script.cc:15: base::StaticAtomicSequenceNumber
g_user_script_id_generator;
nit: maybe a comment on why this can't just be an int(64).

[Mark Dittmer, 2014-08-22 18:28:56]

Addressed nit.

https://codereview.chromium.org/495853002/diff/40001/extensions/common/user_s...
File extensions/common/user_script.cc (right):

https://codereview.chromium.org/495853002/diff/40001/extensions/common/user_s...
extensions/common/user_script.cc:15: base::StaticAtomicSequenceNumber
g_user_script_id_generator;
On 2014/08/22 18:24:21, Devlin wrote:
> nit: maybe a comment on why this can't just be an int(64).

Done.

[Mark Dittmer, 2014-08-22 18:37:28]

The CQ bit was checked by markdittmer@chromium.org

[commit-bot: I haz the power, 2014-08-22 18:38:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/markdittmer@chromium.org/495853002/80001

[commit-bot: I haz the power, 2014-08-22 20:04:49]

Committed patchset #5 (80001) as 291489