Add deprecation message for AppCache in insecure contexts

Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931
Committed: https://crrev.com/1152e786b74c1bbba74af386eb9757f3f8713add
Cr-Commit-Position: refs/heads/master@{#377463}

[jww, 2016-02-23 20:03:14]

Description was changed from

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages.

BUG=588931
==========

to

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931
==========

[jww, 2016-02-23 20:04:19]

jww@chromium.org changed reviewers:
+ yoav@yoav.ws

[jww, 2016-02-23 20:04:20]

Yoav, you like things being deprecated, right? You mind taking a look at this?
Thanks :-)

[jww, 2016-02-23 20:05:05]

Description was changed from

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931
==========

to

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931
==========

[Yoav Weiss, 2016-02-23 22:09:55]

The only thing I love more than deprecations is AppCache deprecations! :)

All in all, looks good. A few comments in the code. 
Can you also add expected.txt tests for both manifest and API use, so that we
can make sure the deprecation messages are printed to console?

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:84:
async_test(function() {
So, just to make sure that I understand this test: This is adding appcache here
and makes sure it is working, so that later on, we could make sure that it
stopped working, right?

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/appcache/ApplicationCache.cpp
(right):

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCache.cpp:147: String
insecureOriginMessage;
Can we get rid of the string here?

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
(right):

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:127: if
(document->isSecureContext(insecureOriginMessage)) {
Can't we use this variant of isSecureContext
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
and avoid the useless String?

[michaeln, 2016-02-23 22:26:33]

michaeln@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2016-02-23 22:26:34]

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
(right):

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:119:
void ApplicationCacheHost::selectCacheWithManifest(const KURL& manifestURL)
fyi: This will miss counting/warning for documents that are loaded from an
appcache but don't have an explicit <html manifest=xxx>  attribute.

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:291: //
FIXME: Prod the inspector to update its notion of what cache the page is using.
CacheInfo info = applicationCacheInfo();

I think a test for a non-empty info.m_manifest would catch all documents loaded
from an appcache.

[jww, 2016-02-23 23:51:42]

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:84:
async_test(function() {
On 2016/02/23 22:09:54, Yoav Weiss wrote:
> So, just to make sure that I understand this test: This is adding appcache
here
> and makes sure it is working, so that later on, we could make sure that it
> stopped working, right?

Yes, exactly. It also makes sure we're testing the console messages (which, as
you pointed out, I forgot to add the updated expected.txt for).

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/appcache/ApplicationCache.cpp
(right):

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCache.cpp:147: String
insecureOriginMessage;
On 2016/02/23 22:09:54, Yoav Weiss wrote:
> Can we get rid of the string here?

Done.

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
(right):

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:119:
void ApplicationCacheHost::selectCacheWithManifest(const KURL& manifestURL)
On 2016/02/23 22:26:34, michaeln wrote:
> fyi: This will miss counting/warning for documents that are loaded from an
> appcache but don't have an explicit <html manifest=xxx>  attribute.

Great point. I'm not sure what the correct thing to do is. I'm tempted to leave
it as-is. When we do the actual removal, we'll probably want to explicitly clear
the AppCache on insecure origins, but I feel like the warning only really makes
sense in the context of actual API use.

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:127: if
(document->isSecureContext(insecureOriginMessage)) {
On 2016/02/23 22:09:54, Yoav Weiss wrote:
> Can't we use this variant of isSecureContext
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> and avoid the useless String?

I didn't know about that variant; done.

https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:291: //
FIXME: Prod the inspector to update its notion of what cache the page is using.
On 2016/02/23 22:26:34, michaeln wrote:
> CacheInfo info = applicationCacheInfo();
> 
> I think a test for a non-empty info.m_manifest would catch all documents
loaded
> from an appcache.

Let me know what you think about my comment above. I'm of the opinion that a
console warning without an actual API use is, at best, confusing.

[Yoav Weiss, 2016-02-24 08:20:09]

On 2016/02/23 23:51:42, jww wrote:
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
> (right):
> 
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:84:
> async_test(function() {
> On 2016/02/23 22:09:54, Yoav Weiss wrote:
> > So, just to make sure that I understand this test: This is adding appcache
> here
> > and makes sure it is working, so that later on, we could make sure that it
> > stopped working, right?
> 
> Yes, exactly. It also makes sure we're testing the console messages (which, as
> you pointed out, I forgot to add the updated expected.txt for).
> 
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/appcache/ApplicationCache.cpp
> (right):
> 
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/appcache/ApplicationCache.cpp:147:
String
> insecureOriginMessage;
> On 2016/02/23 22:09:54, Yoav Weiss wrote:
> > Can we get rid of the string here?
> 
> Done.
> 
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
> (right):
> 
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:119:
> void ApplicationCacheHost::selectCacheWithManifest(const KURL& manifestURL)
> On 2016/02/23 22:26:34, michaeln wrote:
> > fyi: This will miss counting/warning for documents that are loaded from an
> > appcache but don't have an explicit <html manifest=xxx>  attribute.
> 
> Great point. I'm not sure what the correct thing to do is. I'm tempted to
leave
> it as-is. When we do the actual removal, we'll probably want to explicitly
clear
> the AppCache on insecure origins, but I feel like the warning only really
makes
> sense in the context of actual API use.
> 
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:127:
if
> (document->isSecureContext(insecureOriginMessage)) {
> On 2016/02/23 22:09:54, Yoav Weiss wrote:
> > Can't we use this variant of isSecureContext
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > and avoid the useless String?
> 
> I didn't know about that variant; done.
> 
>
https://codereview.chromium.org/1723353002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:291:
//
> FIXME: Prod the inspector to update its notion of what cache the page is
using.
> On 2016/02/23 22:26:34, michaeln wrote:
> > CacheInfo info = applicationCacheInfo();
> > 
> > I think a test for a non-empty info.m_manifest would catch all documents
> loaded
> > from an appcache.
> 
> Let me know what you think about my comment above. I'm of the opinion that a
> console warning without an actual API use is, at best, confusing.

LGTM core/ assuming Michael is happy with the AppCache changes and comment.

[michaeln, 2016-02-24 19:17:47]

> LGTM core/ assuming Michael is happy with the AppCache changes and comment.

lgtm 2, just wanted to make you were aware feature usage vs api usage

[jww, 2016-02-24 19:20:43]

On 2016/02/24 19:17:47, michaeln wrote:
> > LGTM core/ assuming Michael is happy with the AppCache changes and comment.
> 
> lgtm 2, just wanted to make you were aware feature usage vs api usage

Yup, thanks!

[jww, 2016-02-24 19:21:50]

jww@chromium.org changed reviewers:
+ asvitkine@google.com

[jww, 2016-02-24 19:22:42]

asvitkine@, can you review tools/metrics/rappor? Thanks!

[jww, 2016-02-24 19:25:47]

On 2016/02/24 19:22:42, jww wrote:
> asvitkine@, can you review tools/metrics/rappor? Thanks!

My apologies, can you actually review all of tools/metrics? Thanks.

[jww, 2016-02-24 19:26:06]

jww@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[Alexei Svitkine (slow), 2016-02-25 00:14:39]

lgtm

[jww, 2016-02-25 00:19:58]

The CQ bit was checked by jww@chromium.org

[jww, 2016-02-25 00:19:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, yoav@yoav.ws, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1723353002/#ps60001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2016-02-25 00:22:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1723353002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1723353002/60001

[commit-bot: I haz the power, 2016-02-25 02:01:38]

Description was changed from

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931
==========

to

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931
==========

[commit-bot: I haz the power, 2016-02-25 02:01:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-25 02:03:00]

Description was changed from

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931
==========

to

==========
Add deprecation message for AppCache in insecure contexts

This adds a deprecation messages for uses of the manifest attribute or
use of the AppCache API in insecure contexts. Adds LayoutTests as well
to confirm warning messages. Additionally, adds counts and RAPPOR stats
to track usage.

BUG=588931

Committed: https://crrev.com/1152e786b74c1bbba74af386eb9757f3f8713add
Cr-Commit-Position: refs/heads/master@{#377463}
==========

[commit-bot: I haz the power, 2016-02-25 02:03:01]

Patchset 4 (id:??) landed as
https://crrev.com/1152e786b74c1bbba74af386eb9757f3f8713add
Cr-Commit-Position: refs/heads/master@{#377463}

[jochen (gone - plz use gerrit), 2016-02-29 15:44:41]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-02-29 15:44:42]

https://codereview.chromium.org/1723353002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
(right):

https://codereview.chromium.org/1723353002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:130:
UseCounter::countCrossOriginIframe(*document,
UseCounter::ApplicationCacheManifestSelectInsecureOrigin);
This hits ASSERT(Deprecation::deprecationMessage(feature).isEmpty()) if
UseCounter::count()

You'll need to add a new a corresponding method on Deprecation

[jww, 2016-03-01 19:55:02]

https://codereview.chromium.org/1723353002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
(right):

https://codereview.chromium.org/1723353002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:130:
UseCounter::countCrossOriginIframe(*document,
UseCounter::ApplicationCacheManifestSelectInsecureOrigin);
On 2016/02/29 15:44:42, jochen wrote:
> This hits ASSERT(Deprecation::deprecationMessage(feature).isEmpty()) if
> UseCounter::count()
> 
> You'll need to add a new a corresponding method on Deprecation
>  

Yikes, thanks. It appears that I also did this for Geolocation, so I'll fix that
as well.

[playmate113, 2016-06-17 22:16:01]

On 2016/03/01 19:55:02, jww wrote:
>
https://codereview.chromium.org/1723353002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp
> (right):
> 
>
https://codereview.chromium.org/1723353002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/appcache/ApplicationCacheHost.cpp:130:
> UseCounter::countCrossOriginIframe(*document,
> UseCounter::ApplicationCacheManifestSelectInsecureOrigin);
> On 2016/02/29 15:44:42, jochen wrote:
> > This hits ASSERT(Deprecation::deprecationMessage(feature).isEmpty()) if
> > UseCounter::count()
> > 
> > You'll need to add a new a corresponding method on Deprecation
> >  
> 
> Yikes, thanks. It appears that I also did this for Geolocation, so I'll fix
that
> as well.