Running local server on non-localhost causes breakages

[bradfrizzell]

Steps to reproduce:

gulp --host=<insert name of host machine>

navigate to hostname:8000/examples/ads.amp.html
Select doubleclick for ad network.
Notice how all the ads 404

The issue is that non-localhost is not being identified as localDev in mode.js, which is instead hardcoding localhost in its check for localdev mode.

[erwinmombay]

looking

[rsimha]

I believe this is stemming from the hardcoded check for localhost here: https://github.com/ampproject/amphtml/blob/master/src/mode.js#L79

[rsimha]

Another option is to force localDev via the mechanism here: https://github.com/ampproject/amphtml/blob/master/src/mode.js#L75 (Not sure where in gulp watch or gulp serve this can be inserted, though.)

[bradfrizzell]

this also complicates it: https://github.com/ampproject/amphtml/blob/master/src/3p-frame.js#L235

[rsimha]

@erwinmombay Are you working on a fix? If not, I'm happy to pick this up.

[rsimha]

@bradfrizzell @erwinmombay I just sent out a fix for this in #12152. Once merged, the following will no longer result in 404s when you load hostname.domain.com:8000/examples/ads.amp.html:

gulp clean
export AMP_TESTING_HOST=<hostname.domain.com>
gulp build
gulp serve --host=<hostname.domain.com>

Note that for the ads to actually be served in this scenario, you'll probably have to change how __amp_source_origin works, but I'll leave it to the a4a team to determine the behavior.

[rsimha]

After merging #12152, a couple of hidden bugs in examples/ads.amp.html have been exposed:

1. When localDev is set to true in the AMP runtime, the page looks like this:

2. When localDev is set to true, and thirdPartyFrameHost and thirdPartyFrameRegex are set to a non-localhost machine name, the page looks like this:

A workaround for 1 is to run these commands, after which the page will load correctly:

gulp build
gulp prepend-global --target dist/amp.js --{prod|canary}
gulp serve

@bradfrizzell assigning this back to you for further investigation.

[rsimha]

I have a fix (for item 1 in #12054 (comment)) out for review at #12458

[rsimha]

@bradfrizzell Assigning this back to you for item 2 in #12054 (comment), where localDev is true, and the ads are being served from a non-localhost server. Let me know what you find out.

[lannka]

@rsimha-amp shall we completely remove any hardcoded localhost check in AMP (like here)? Instead, we can have our local server to prepend "AMP_CONFIG.localDev = true" to the served binary.

That makes sure AMP runtime can be run under any domain in dev mode, which is sometimes very helpful if we want to locally fake the CDN domain and test CORS related logic (like A4A fast fetch).

[rsimha]

@lannka I originally removed the localhost check you pointed to, but it had to be restored in #11783 by @erwinmombay because it broke some of our test pages.

I do agree though, that it makes sense to simplify the logic in mode.js.

[lannka]

  const FORCE_LOCALDEV = !!(self.AMP_CONFIG && self.AMP_CONFIG.localDev);
  const AMP_CONFIG_3P_FRAME_HOST = self.AMP_CONFIG &&
      self.AMP_CONFIG.thirdPartyFrameHost;
  const isLocalDev = IS_DEV 
      && !!(win.location.hostname == 'localhost' 
          || (FORCE_LOCALDEV && win.location.hostname == AMP_CONFIG_3P_FRAME_HOST) 
          || (win.location.ancestorOrigins && win.location.ancestorOrigins[0] && startsWith(win.location.ancestorOrigins[0], 'http://localhost:'))
      )
      &&
      // Filter out localhost running against a prod script.
      // Because all allowed scripts are ours, we know that these can only
      // occur during local dev.
      (!win.document || !!win.document.querySelector(developmentScriptQuery));

This is the current isLocalDev logic.
@erwinmombay can you talk about the necessity of all those conditions?

I want to simplify this to:

 const isLocalDev = IS_DEV && FORCE_LOCALDEV;

And let our gulp build or gulp server to always append AMP_CONFIG.isLocalDev = true

[erwinmombay]

@lannka i honestly don't remember why we need all the location checks (probly a time when we didnt have force local dev yet and even no amp config yet). but if we can make it simpler, go for it.

[lannka]

Thanks @erwinmombay .

I tested the proposed change, once we prepend AMP_CONFIG to amp.js, the ads examples start to work.

However, right now I have to do:

gulp build --config=canary
gulp serve

It does not work if I ran the local server using:

gulp

@rsimha-amp can we make build with "--config=canary" the default?

[rsimha]

We use --config=prod by default. Why does that not work? What would it take to make that work too?

[rsimha]

@lannka do both sets of steps in #12458 (comment) work with your change? What about the steps in #12054 (comment)? And what about Heroku? I believe some of those conditions were put in to make all these scenarios work.

I like your fix though, so let me know how I can help to make it real :)

[lannka]

We use --config=prod by default. Why does that not work? What would it take to make that work too?

I think --config=prod is not turned on when running gulp. I looked at the output at dist/amp.js, it does not have the AMP_CONFIG prepend.

I think the fix should be in gulpfile.js. Currently the default gulp command is an alias of gulp watch, we might have gulp watch to support --config=prod.

[rsimha]

@lannka Good catch. Sent you #12665 to fix the behavior. With this, running gulp or gulp watch will enable localDev by default.

[rsimha]

@lannka Now that gulp and gulp watch also enable localDev in AMP_CONFIG by default, I think the coast is clear to simplify the isLocalDev logic as you suggested. Sent you #12667 with the fix. PTAL.

[rsimha]

@bradfrizzell Now that the localDev logic is simplified, there is only one item left to sort out. It turns out that when you run...

export AMP_TESTING_HOST=<hostname>
gulp --host=<hostname>

... and navigate to http://<hostname>:8000/examples/ads.amp.html, the ads don't load because the page is trying to fetch them from http://ads.localhost:8000/dist.3p/current and not http://<hostname>:8000/dist.3p/current.

I think if we change src/3p-frame.js to return <hostname> instead of ads.localhost, the problem originally reported in this issue should be fixed.

Just sent out #12673 with a fix.

[rsimha]

Reopening based on #12666 and #12673 (comment)

[rsimha]

Paging @erwinmombay @lannka @ampproject/a4a :)

After debugging, I'm seeing that validateAllowedTypes() in 3p/integration.js isn't seeing the thirdParty* overrides in self.AMP_CONFIG that get applied when we set AMP_TESTING_HOST. However, getAdsLocalhost() in src/3p-frame.js is correctly seeing them.

In other words, urls.thirdPartyFrameHost in the main window is AMP_TESTING_HOST, while urls.thirdPartyFrameHost in the 3p frame (/dist.3p/current/frame.max.html) is the localhost based default value. This is resulting in the errors:

log.js:324 Error: Non-whitelisted 3p type for custom iframe: a8​​​
   at Log.assert (log.js:318)
   at validateAllowedTypes (integration.js:822)
   at window.draw3p (integration.js:478)
   at frame.max.html:9

Presumably, this is because the AMP config overrides aren't being applied to the 3rd party frame, which is probably why non-localhost scenarios never did work.

Do any of you know of a recommended way to apply testing overrides to AMP_CONFIG such that third party frames use those values too?

[rsimha]

Turns out we need to write AMP_CONFIG to the 3p frame as well, for local development with a non-localhost server. Fix available in #12703.