Refactor experiments to use localStorage #22796

dreamofabear · 2019-06-11T22:08:31Z

Summary

Mitigates cookie tossing between AMP Cache subdomains
Release channel opt-in (dev, RC) is the only remaining usage of cookies on AMP Cache

Caveats

It's no longer possible to enable experiments for multiple publishers on the AMP Experiments page.

For example, to enable experiments for pages with source origin www.bbc.co.uk, need to navigate to https://www-bbc-co-uk.cdn.ampproject.org/experiments.html.

Note that cookie usage was already unreliable on Safari due to ITP2 (#18889).

How to enable experiments

When testing on desktop or tethered mobile devices:

Continue using AMP.toggleExperiment('foo') in dev console (or methods below).

For testing on untethered mobile devices:

For viewer pages [*], enter the source origin into the new input field on cdn.ampproject.org/experiments.html to find the corresponding subdomain experiments page (see screenshot).
For publisher-origin pages, use bookmarklets e.g. add a bookmark with URL javascript:AMP.toggleExperiment('foo').

[*] Due to Safari cache partitioning, experiments can only be enabled via AMP.toggleExperiment in dev tools on Safari.

Screenshots

https://cdn.ampproject.org/experiments.html

https://www-nytimes-com.cdn.ampproject.org/experiments.html

Existing usages of localStorage

Key	Value	Description
"amp-app-banner: element_id"	boolean	Dismissed?
"amp-consent: element_id"	boolean or ConsentInfoDef	User consent state
"amp-user-notification: element_id"	boolean	Dismissed?
"amp-cid-optout"	boolean	Opted out of CID via AMP.optoutOfCid?
"amp-access-iframe"	{t: timestamp, d: authorization_response}	amp-access response
"amp-web-push-notification-permission"	string	window.Notification.permission

dreamofabear · 2019-06-12T22:20:35Z

/to @jridgewell /cc @cramforce @molnarg

cramforce · 2019-06-12T22:26:12Z

src/experiments.js

-  const tokens = experimentCookie ? experimentCookie.split(/\s*,\s*/g) : [];
+function getExperimentToggles(win) {
+  const experimentsString =
+    'localStorage' in win ? win.localStorage.getItem(LOCAL_STORAGE_KEY) : '';


This probably needs to be try-block guarded.

Done, thanks.

cramforce · 2019-06-12T22:31:32Z

tools/experiments/experiments.js

+  const button = redirect.querySelector('button');
+  const anchor = redirect.querySelector('a');
+  button.addEventListener('click', function() {
+    const ampUrl = viewerToAmpUrl(input.value);


I think we should switch to folks just entering their domain and going from there instead of mentioning the AMP viewer.

How about removing the viewer mention but we detect and support both?

Copy/pasting a viewer URL avoids typing.

That might suggest it would work in Safari (which it won't). I'd really go with simple here.

Sorry, which part won't work in Safari?

Viewer URL detection is just "is the domain google.com?". We can even add support for Bing viewer here, though their experiments page is currently broken. :)

Anyways I removed it, but some mobile testing workflows will suffer.

In Safari, localStorage in a top-window (cache URL) isn't visible when the same origin loads in a viewer.

TIL 🤦‍♂

dreamofabear · 2019-06-12T22:41:11Z

Re: #22247 (comment)

We should definitely save all experiments as a single localStorage key.

@cramforce Done, and noted to switch to KV storage when available.

can we include this in the Chrome extension? (the one with the validator in it)

@aghassemi I looked into adding something to the AMP Validator Chrome extension but it doesn't seem like a great fit and of limited use since it's desktop only.

I think the bookmarklet trick works on mobile and is almost as good. I can help write the code for it.

take advantage of new error messaging stuff that points to amp.dev with more in-depth explanations (runtime message in dev console)

@kristoferbaxter Changed AMP.toggleExperiment to output a user warning:

"foo" experiment enabled for the domain "www-example-com.cdn.ampproject.org". See: https://amp.dev/documentation/guides-and-tutorials/learn/experimental

aghassemi · 2019-06-12T23:03:23Z

@aghassemi I looked into adding something to the AMP Validator Chrome extension but it doesn't seem like a great fit and of limited use since it's desktop only.

I think the bookmarklet trick works on mobile and is almost as good. I can help write the code for it.

This is going to be too painful for experiments that impact all pages during dogfood phase. When we were implementing image autolighbox, the whole UI team had the experiment on and we were constantly trying it on AMP pages we were reading as part of our day and reporting bugs.

New Loaders is another one that we want to enable the experiment manually for a few weeks on personal devices to find issues. If one has to click on a bookmark or copy paste a Url everytime, the experiment just won't get the same exposure.

Integrating with a Chrome extension at least helps a bit when dogfooding on desktop.

Is this really worth doing given the restrictions it creates? Is cookie tossing possible without our JS or CDN being compromised?

dreamofabear · 2019-06-12T23:11:24Z

Makes sense, I'll see if I can hack something up.

cramforce · 2019-06-12T23:20:30Z

@aghassemi Cookie will break anyway, when cdn.ampproject.org goes on the suffix list. If you want to do horizontal experiments going forward, it'll need either a viewer experiment or a chrome extension.

aghassemi · 2019-06-13T02:53:36Z

What about @jridgewell's solution? Make the experiment page set http-only cookie and have serving infra amend the JS it serves with the right flags?

cramforce · 2019-06-13T03:42:21Z

Cookies will have the same scope as localStorage when we switch to having cdn.ampproject.org on the suffix list.

…

On Wed, Jun 12, 2019 at 7:53 PM Ali Ghassemi ***@***.***> wrote: What about @jridgewell <https://github.com/jridgewell>'s solution? Make the experiment page set http-only cookie and have serving infra amend the JS it serves with the right flags? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22796?email_source=notifications&email_token=AAAV4T4KKWNVEY5GZPEDWVDP2GZDDA5CNFSM4HXDX3Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXSLQOI#issuecomment-501528633>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAV4T2XR4BHCKZCURDA3ZDP2GZDDANCNFSM4HXDX3YQ> .

aghassemi · 2019-06-13T05:20:30Z

@cramforce So the actual suffix domain itself can't have its own cookie jar? Unless that's the case (which is surprising to me because domains like js.org are listed but it seems to set cookies on itself just fine), then If https://cdn.ampproject.org/experiment.html sets a http-only cookie for cdn.ampproject.org from a direct user visit, why can't the server serving https://cdn.ampproject.org/v0.js for a <script> in 3p context read that cookie? (regardless of whether the life of that cookie is shortened by ITP or not which IDK if it happens)

dreamofabear · 2019-06-13T16:49:43Z

Huh true, public suffixes can set cookies for themselves despite what the spec implies: publicsuffix/list#461

We'll likely move to public suffix eventually but I wanted to postpone it for now since it takes O(months) for browsers to update (and then it only affects latest version). It also breaks canary opt-in for CDN pages which is an even bigger dev-ex pain.

cramforce · 2019-06-13T19:46:45Z

Extended suffixes may be able to set cookies for themselves (which would be cool, cause it would mean we would not lose the dev channel opt-in!!!) but they cannot set cookies that are visible to their children.

aghassemi · 2019-06-13T21:04:34Z

So, could we please make the Experiments work similar to Dev/RC channel and have the server respond with patched JS? I understand this will require new changes on the serving infra, but being able to turn on an experiment for all AMP is really really valuable.

dreamofabear · 2019-06-14T15:49:43Z

We wouldn't lose opt-in for pages that use unversioned JS URLs, but we'd lose it for CDN pages which reads the parent domain cookie for RTV rewriting.

So we'd need two cache serving changes:

Experiment cookie causes server-side changes of v0.js instead of reading at runtime
Make opt-in work for versioned URLs the same way as unversioned

Like I said, happy to pursue these changes (which may be non-trivial) as part of the move to public suffix list.

However for short-term, I'd still like to get this in for immediate security benefit and to unblock an important launch. :)

aghassemi · 2019-06-14T15:55:35Z

However for short-term, I'd still like to get this in for immediate security benefit and to unblock an important launch. :)

I don't agree with breaking a very useful feature to get things launched without doing the complete solution first, but that is not my call.

dreamofabear · 2019-06-14T16:03:54Z

Appreciate the candid feedback. It also pointed out a better long-term solution which is great.

As a counterpoint, this seems like a (temporary) trade off of dev-ex for user experience:

User Experience > Developer Experience > Ease of Implementation

aghassemi · 2019-06-14T16:16:02Z

Sorry I am holding a hard position on this, ability to opt-in to experiments across all AMP has saved us tremendously in the past to ensure we launch higher quality horizontal features such as auto lightbox, auto-sizes for amp-img. A significant number of experiments in https://cdn.ampproject.org/experiments.html are horizontal experiments.

Currently this is not a (temporary) trade off of dev-ex for user experience IMO since the "user experience" in question here is an unlaunched feature. It seems like a trade of dev-ex (which in this case is about quality assurance to ensure good UX anyway) for a quicker launch.

cramforce · 2019-06-14T16:17:48Z

The horizontal experiments only work on the AMP Cache, where they can also be turned on via URL param (and a Search experiment), so I think this is independent.

aghassemi · 2019-06-14T18:48:36Z

The horizontal experiments only work on the AMP Cache

Right now yes, the proposed approach will also fix that. You also know how much of a pain setting up a Search experiment is. Can we at least do an assessment of how much work serving infra needs to do to make experiments work like RC/Dev? I think we all agree with the long-term solution, question is whether the short-term solution is a good tradeoff. I see the trade-off as putting burden of work on Runtime's users to go through more work to setup experiments to test their code vs launch of amp-script. If the work needed on serving infra is like ~1 week, then I would selfishly say please don't break this feature on us. If it's a month of work, I understand.

dreamofabear · 2019-06-14T19:26:37Z

the "user experience" in question here is an unlaunched feature

This plugs an existing potential security hole for AMP Cache almost immediately, which is mostly independent of amp-script.

Also, horizontals are still possible for experiments enabled in canary-config.json, and 0.5% of users is within norms e.g. for Search LEs. So the quantified dev-ex impact is "somewhat greater friction in low-volume alpha testing across many domains".

If it's a month of work, I understand.

I'll start investigating this. But Chrome pulls PSL as part of release cycle (every 6 weeks) and Safari is possibly much longer.

jridgewell

I agree with @aghassemi that this will be annoying for horizontal experiments. If we can get approval for a google.com cookie, I can make this much easier by using query params to enable-disable the cookie during the foo-com.cdn.ampproject.org document request. But this won't really help origin pages.

jridgewell · 2019-06-14T21:36:32Z

src/experiments.js

+      if (!getMode().test) {
+        user().warn(
+          TAG,
+          '"%s" experiment %s for the domain "%s". See: https://amp.dev/documentation/guides-and-tutorials/learn/experimental',


Nit: Enabled/Disabled experiment "foo" on domain "bar.com"

jridgewell · 2019-06-14T21:37:06Z

src/experiments.js

-  const tokens = experimentCookie ? experimentCookie.split(/\s*,\s*/g) : [];
+function getExperimentToggles(win) {
+  let experimentsString = '';
+  if ('localStorage' in win) {


I think even testing in causes an error in Safari. Let's move this inside the try-catch.

jridgewell · 2019-06-14T21:38:10Z

src/experiments.js

-      // Set explicit domain, so the cookie gets send to sub domains.
-      domain: win.location.hostname,
-      allowOnProxyOrigin: true,
+  if ('localStorage' in win) {


jridgewell · 2019-06-14T21:39:04Z

tools/experiments/experiments.js

+  if (host === 'cdn.ampproject.org') {
+    const experimentsDesc = document.getElementById('experiments-desc');
+    experimentsDesc.setAttribute('hidden', '');
+    experimentsTable.setAttribute('hidden', '');


Should we be displaying an error message?

There's a new message in the UI (see screenshot). Or did you mean dev console?

* Refactor experiments to use localStorage, not cookies. * Update experiments.html. * Fix lint. * Whitelist in presubmit check for localStorage. * Show redirect tool on cdn.ampproject.org instead of experiment toggles. * Use util functions where possible. * Add user warning to AMP.toggleExperiment. * Update experiments/README.md. * Fix tests. * Try/catch localStorage, support origin URLs. * Remove viewer URL support. * Move localStorage check inside try/catch.

William Chou added 2 commits June 11, 2019 17:04

Refactor experiments to use localStorage, not cookies.

21f7219

Update experiments.html.

74fb4b5

googlebot added the cla: yes label Jun 11, 2019

Fix lint.

9a14818

dreamofabear mentioned this pull request Jun 11, 2019

Design Review 2019-06-12 15:30 UTC (wg-runtime, Cloudinary extension, localStorage for experiments) #22247

Closed

William Chou added 5 commits June 11, 2019 19:45

Whitelist in presubmit check for localStorage.

bd17447

Show redirect tool on cdn.ampproject.org instead of experiment toggles.

f22b842

Use util functions where possible.

9cb740b

Add user warning to AMP.toggleExperiment.

0b9f472

Update experiments/README.md.

3f0495e

dreamofabear mentioned this pull request Jun 12, 2019

Update docs for cookie->localStorage experiments change ampproject/amp.dev#2306

Merged

Fix tests.

7edbcdf

dreamofabear requested a review from jridgewell June 12, 2019 22:19

cramforce reviewed Jun 12, 2019

View reviewed changes

Try/catch localStorage, support origin URLs.

340ce83

Remove viewer URL support.

4f509e5

dreamofabear force-pushed the experiments-on-local-storage branch from 300a456 to 4f509e5 Compare June 13, 2019 16:59

Merge branch 'master' into experiments-on-local-storage

a5d353d

jridgewell approved these changes Jun 14, 2019

View reviewed changes

William Chou added 2 commits June 20, 2019 12:08

Move localStorage check inside try/catch.

d9f76a2

Merge branch 'master' into experiments-on-local-storage

d4fcc96

jridgewell approved these changes Jul 10, 2019

View reviewed changes

dreamofabear merged commit 419ad60 into ampproject:master Jul 10, 2019

dreamofabear deleted the experiments-on-local-storage branch July 10, 2019 19:07

ampprojectbot mentioned this pull request Apr 27, 2020

🚨 Error: Object.assign requires that input parameter not be null or undefined #28056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor experiments to use localStorage #22796

Refactor experiments to use localStorage #22796

dreamofabear commented Jun 11, 2019 •

edited

dreamofabear commented Jun 12, 2019

cramforce Jun 12, 2019

dreamofabear Jun 13, 2019

cramforce Jun 12, 2019

dreamofabear Jun 12, 2019

cramforce Jun 12, 2019

dreamofabear Jun 13, 2019

cramforce Jun 13, 2019

dreamofabear Jun 14, 2019

dreamofabear commented Jun 12, 2019

aghassemi commented Jun 12, 2019

dreamofabear commented Jun 12, 2019

cramforce commented Jun 12, 2019

aghassemi commented Jun 13, 2019

cramforce commented Jun 13, 2019 via email

aghassemi commented Jun 13, 2019

dreamofabear commented Jun 13, 2019

cramforce commented Jun 13, 2019

aghassemi commented Jun 13, 2019

dreamofabear commented Jun 14, 2019

aghassemi commented Jun 14, 2019

dreamofabear commented Jun 14, 2019

aghassemi commented Jun 14, 2019

cramforce commented Jun 14, 2019

aghassemi commented Jun 14, 2019 •

edited

dreamofabear commented Jun 14, 2019

jridgewell left a comment

jridgewell Jun 14, 2019

jridgewell Jun 14, 2019

jridgewell Jun 14, 2019

jridgewell Jun 14, 2019

dreamofabear Jun 20, 2019

Refactor experiments to use localStorage #22796

Refactor experiments to use localStorage #22796

Conversation

dreamofabear commented Jun 11, 2019 • edited

Summary

Caveats

How to enable experiments

Screenshots

Existing usages of localStorage

dreamofabear commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Jun 12, 2019

aghassemi commented Jun 12, 2019

dreamofabear commented Jun 12, 2019

cramforce commented Jun 12, 2019

aghassemi commented Jun 13, 2019

cramforce commented Jun 13, 2019 via email

aghassemi commented Jun 13, 2019

dreamofabear commented Jun 13, 2019

cramforce commented Jun 13, 2019

aghassemi commented Jun 13, 2019

dreamofabear commented Jun 14, 2019

aghassemi commented Jun 14, 2019

dreamofabear commented Jun 14, 2019

aghassemi commented Jun 14, 2019

cramforce commented Jun 14, 2019

aghassemi commented Jun 14, 2019 • edited

dreamofabear commented Jun 14, 2019

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Jun 11, 2019 •

edited

aghassemi commented Jun 14, 2019 •

edited