Fix issue that causes some extensions to be disabled right after installation.

Fix issue that causes some extensions to be disabled right after installation.
Add an option to Extension::GetDistinctHosts that strips the RCDs off the hosts.

BUG=65138
TEST=ExtensionsServiceTest, ExtensionTest

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

[jstritar, 2010-12-03 17:23:59]

Erik, could you take a look at this one related to Extension::GetDistinctHosts?
Thanks

[Erik does not do reviews, 2010-12-03 17:56:42]

Great catch.  Bummer that my tests didn't catch this.

Here's another idea for tackling this.  Rather than having a parameter like
this, we could make this method private and have two wrapper functions:

GetDistinctHostsForDisplay(URLPatternList& list)
IsElevatedHostList(URLPatternList& old, URLPatternList& new)

With this, you could also add a test I didn't see in your change which changed
the order of the same list and compared them.

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:356: // Include the RCDs in the distinct
hosts to make it easier on the users.
buff this comment out a bit:
"...to make it easier for the users to read.  For example, this will display
'*.google.co.uk' rather than '*.google.'"

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:130: // and de-dupe the result.
...the result, including based on filtering out common hosts with differing
RCDs.

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:131: // NOTE: If using the hosts for
privilege comparisons, |include_rcd| must be
Rather than explaining it as a NOTE like this, I'd just explain the parameter's
two use cases:
If |include_rcd| is true, then the de-duped result will be the first full entry,
including its RCD.  So if the list was *.google.co.uk and *.google.com, the
returned value would just be *.google.co.uk.  Keeping the RCD in the result is
useful for display purposes when you want to show the user one sample hostname
from the list.  If you need to compare two URLPatternLists for security
equality, then set |include_rcd| to false, which will return a result like
"*.google.", regardless of the order of the hosts.

[jstritar, 2010-12-03 21:52:55]

Using GetDistinctHostsForDisplay and IsElevatedHostList worked out nicely...
thanks. Uploaded a new CL that incorporates the feedback.

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:356: // Include the RCDs in the distinct
hosts to make it easier on the users.
On 2010/12/03 17:56:42, Erik Kay wrote:
> buff this comment out a bit:
> "...to make it easier for the users to read.  For example, this will display
> '*.google.co.uk' rather than '*.google.'"

I ended up removing this comment after switching to GetDistinctHostsForDisplay.

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:130: // and de-dupe the result.
On 2010/12/03 17:56:42, Erik Kay wrote:
> ...the result, including based on filtering out common hosts with differing
> RCDs.

Done.

http://codereview.chromium.org/5642001/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:131: // NOTE: If using the hosts for
privilege comparisons, |include_rcd| must be
On 2010/12/03 17:56:42, Erik Kay wrote:
> Rather than explaining it as a NOTE like this, I'd just explain the
parameter's
> two use cases:
> If |include_rcd| is true, then the de-duped result will be the first full
entry,
> including its RCD.  So if the list was *.google.co.uk and *.google.com, the
> returned value would just be *.google.co.uk.  Keeping the RCD in the result is
> useful for display purposes when you want to show the user one sample hostname
> from the list.  If you need to compare two URLPatternLists for security
> equality, then set |include_rcd| to false, which will return a result like
> "*.google.", regardless of the order of the hosts.

Done.

[Erik does not do reviews, 2010-12-03 23:40:40]

LGTM

+cnygaard - FYI, the host logic had some bugs that may affect your
implementation as well

http://codereview.chromium.org/5642001/diff/11001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/5642001/diff/11001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:330: // considered an elevation, even
though it is not (http://crbug.com/65337).
Good point.  This seems straightforward to fix, although I agree it should wait
for another CL.  I think we'd want to add a "MatchesPattern()" method to
URLPattern to test to see if a pattern is a strict superset of another.  As-is,
there isn't a direct way to do this.

http://codereview.chromium.org/5642001/diff/11001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:356: // Add the subdomain wildcard back to
the host, if necessary.
ah, I see how this happened now.

http://codereview.chromium.org/5642001/diff/11001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/5642001/diff/11001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:131: // hosts with differing RCDs.
add (NOTE: when de-duping hosts with common RCDs, the first pattern is returned
and the rest discarded)

[Aaron Boodman, 2010-12-13 02:06:47]

http://codereview.chromium.org/5642001/diff/17001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/5642001/diff/17001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:362: candidate = "*." + candidate;
Why was this change necessary? It causes a manifest that contains:

"*.google.com",
"google.com"

... to have two entries in the dialog. Granted, it is weird for developers to do
that.

[jstritar, 2010-12-13 16:26:52]

On 2010/12/13 02:06:47, Aaron Boodman wrote:
>
http://codereview.chromium.org/5642001/diff/17001/chrome/common/extensions/ex...
> File chrome/common/extensions/extension.cc (right):
> 
>
http://codereview.chromium.org/5642001/diff/17001/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:362: candidate = "*." + candidate;
> Why was this change necessary? It causes a manifest that contains:
> 
> "*.google.com",
> "google.com"
> 
> ... to have two entries in the dialog. Granted, it is weird for developers to
do
> that.

I did that to make the implementation more consistent with its documentation,
and so that you could not add sub-domains without prompting the user (like going
from "google.com" to "*.google.com" ... or maybe we want to be able to do
that?). I created a bug related to this, http://crbug.com/65337, but I hadn't
thought about it affecting the install prompts.

I think we should treat *.google.com and google.com separately for permissions,
but  just not display google.com since its contained within *.google.com.
Basically, we can add URLPattern::MatchesPattern and then ignore patterns that
are subsets of other patterns. Is this something I should fix for M9?

[Aaron Boodman, 2010-12-13 17:13:17]

On 2010/12/13 16:26:52, jstritar1 wrote:
> On 2010/12/13 02:06:47, Aaron Boodman wrote:
> >
>
http://codereview.chromium.org/5642001/diff/17001/chrome/common/extensions/ex...
> > File chrome/common/extensions/extension.cc (right):
> > 
> >
>
http://codereview.chromium.org/5642001/diff/17001/chrome/common/extensions/ex...
> > chrome/common/extensions/extension.cc:362: candidate = "*." + candidate;
> > Why was this change necessary? It causes a manifest that contains:
> > 
> > "*.google.com",
> > "google.com"
> > 
> > ... to have two entries in the dialog. Granted, it is weird for developers
to
> do
> > that.
> 
> I did that to make the implementation more consistent with its documentation,
> and so that you could not add sub-domains without prompting the user (like
going
> from "google.com" to "*.google.com" ... or maybe we want to be able to do
> that?). I created a bug related to this, http://crbug.com/65337, but I hadn't
> thought about it affecting the install prompts.

I see. I think that maybe going from *.google.com to google.com and back should
be OK. To the user, we display both as 'access to google.com'.

> I think we should treat *.google.com and http://google.com separately for
permissions,
> but  just not display http://google.com since its contained within
*.google.com.
> Basically, we can add URLPattern::MatchesPattern and then ignore patterns that
> are subsets of other patterns.

The problem is that then we would re-prompt users even when, from their point of
view, nothing changed (they get the same warnings they got before).

> Is this something I should fix for M9?

No, I think it will be rare for developers to do this. I only noticed it because
we had a test that did it.