Protect the entire webstore origin, not just the webstore path, with XFO. Follow-up to crrev.com/1…

Protect the entire webstore origin, not just the webstore path, with XFO.  Follow-up to crrev.com/157802 to close a hole pointed out by cevans@

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

[Nico, 2012-09-21 02:22:16]

sounds like it'd be good if there was a test for this

[Chris Evans, 2012-09-21 02:44:27]

LGTM

Satisfies my complaint.

[Tom Sepez, 2012-09-21 17:48:00]

Nico, I've added a test that flunks without the new patch.  Thanks.

[commit-bot: I haz the power, 2012-09-21 22:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10959032/5002

[commit-bot: I haz the power, 2012-09-21 22:24:38]

Presubmit check for 10959032-5002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/extensions/api/webstore_private

[Nico, 2012-09-22 02:09:36]

https://chromiumcodereview.appspot.com/10959032/diff/5002/chrome/browser/exte...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://chromiumcodereview.appspot.com/10959032/diff/5002/chrome/browser/exte...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:281:
FLAKY_FrameErrorPageBlocked) {
Adding a new test as FLAKY_ seems pretty pointless. Can you try to make it not
flaky?

[Tom Sepez, 2012-09-22 21:15:16]

The test is only theoretically flakey, as I've not seen it fail in practice. 
Even so, adding a flaky test doesn't seem pointless to me.  If a test passes
with 90% probability and you see a dozen failures in a row, then you can be
pretty darn sure you've broken something.
 
As the comment in the test file indicates, it can't be made 100% non-flaky until
the underlying webkit bug is fixed.  Without it, there is no event upon which to
terminate the test and we have to resort to a polling model with a timeout,
where hitting the timeout implies failure.  This can't be distinguished from the
slow server case, hence the FLAKY_.  I doubt that it will be an issue.

[Nico, 2012-09-24 00:32:44]

On 2012/09/22 21:15:16, Tom Sepez wrote:
> The test is only theoretically flakey, as I've not seen it fail in practice.

Since it's marked as FLAKY_, the bots won't catch it if it starts failing more
often.
 
> Even so, adding a flaky test doesn't seem pointless to me.  If a test passes
> with 90% probability and you see a dozen failures in a row, then you can be
> pretty darn sure you've broken something.

Yes, but nobody looks at that. We've had FLAKY_ for a while because of this and
other arguments, but in practice this didn't work out well and we mass-converted
all FLAKY_ tests to DISABLED_ about 8 months ago.

> As the comment in the test file indicates, it can't be made 100% non-flaky
until
> the underlying webkit bug is fixed.  Without it, there is no event upon which
to
> terminate the test and we have to resort to a polling model with a timeout,
> where hitting the timeout implies failure.  This can't be distinguished from
the
> slow server case, hence the FLAKY_.  I doubt that it will be an issue.

Can you add a link to the webkit bug to the _apitest file? Do you have an eta
when this will be resolved?

[Tom Sepez, 2012-09-24 17:10:07]

> we mass-converted all FLAKY_ tests to DISABLED_ about 8 months ago.
Ok. Removed the FLAKY_ prefix and hope for the best.

> Can you add a link to the webkit bug to the _apitest file? 
Done.

> Do you have an eta when this will be resolved?
Resolution is still hung up on agreement on what the right behaviour should be.

[Nico, 2012-09-24 22:36:02]

lgtm

[commit-bot: I haz the power, 2012-09-24 22:38:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10959032/15001

[commit-bot: I haz the power, 2012-09-25 00:12:38]

Change committed as 158453