sanitizeHtml in src/sanitizer.js is causing visual diff JS errors (started with prod push)

[rsimha]

See discussion at #13333 (comment)

This started failing on master with this error:

/home/travis/.rvm/gems/ruby-2.4.1/gems/poltergeist-1.17.0/lib/capybara/poltergeist/browser.rb:384:in `command': One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details). (Capybara::Poltergeist::JavascriptError)
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
Removing "class" attribute with invalid value in <amp-img class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt">.
    at https://cdn.ampproject.org/v0.js:2 in pa
	from /home/travis/.rvm/gems/ruby-2.4.1/gems/poltergeist-1.17.0/lib/capybara/poltergeist/browser.rb:39:in `visit'
	from /home/travis/.rvm/gems/ruby-2.4.1/gems/poltergeist-1.17.0/lib/capybara/poltergeist/driver.rb:99:in `visit'
	from /home/travis/.rvm/gems/ruby-2.4.1/gems/capybara-2.17.0/lib/capybara/session.rb:274:in `visit'
	from build-system/tasks/visual-diff.rb:288:in `block (2 levels) in generate_snapshots'
	from build-system/tasks/visual-diff.rb:281:in `each'
	from build-system/tasks/visual-diff.rb:281:in `block in generate_snapshots'
	from build-system/tasks/visual-diff.rb:275:in `each'
	from build-system/tasks/visual-diff.rb:275:in `generate_snapshots'
	from build-system/tasks/visual-diff.rb:250:in `run_visual_tests'
	from build-system/tasks/visual-diff.rb:433:in `main'
	from build-system/tasks/visual-diff.rb:441:in `<main>'

[rsimha]

/cc @choumx

[rsimha]

The root cause of this bug isn't clear, so let's disable the test for now.

[dreamofabear]

Thanks for finding this.

[dreamofabear]

Ah shoot, this is caused by a new user error introduced in #13176.

[dreamofabear]

Timing most likely related to the 1% to prod promotion today: https://github.com/ampproject/amphtml/releases/tag/1517876307637

Remind me if our visual diffing is supposed to use prod binaries?

[rsimha]

@choumx It's supposed to use local binaries with the canary and prod configs applied to it one after another. Clearly, something must have changed with the test pages in examples/visual_tests, causing the test to fetch the prod AMP binary.

I'm making some wholesale changes to the visual diff infra as we speak and will investigate, but meanwhile, let's disable the failing test while we investigate. SG?

[rsimha]

@choumx I just ran gulp visual-diff --webserver_debug to see the logs from the local webserver that's started in order to serve the AMP runtime. The visual tests are indeed pulling the test pages from the webserver, and the test pages are pulling /dist/amp.js from the local webserver.

See https://gist.github.com/rsimha-amp/04cbc1581e3c593d29f5538422afbed8

I'm puzzled about why a prod push would cause this local test to start failing. Could it be just because we apply the canary and prod configs to the local runtime with gulp prepend-global?

/cc @erwinmombay

(Edited for correctness)

[dreamofabear]

From your link it appears that the served JS is also local.

GET /examples/visual-tests/amp-list/amp-list.amp.html 200 1.907 ms - 2460
GET /dist/v0/amp-mustache-0.1.max.js 200 3.207 ms - 224023
GET /dist/v0/amp-list-0.1.max.js 200 5.413 ms - 268702
GET /dist/amp.js 200 2.101 ms - 1553784

[rsimha]

Yeah, I'm puzzled. Could one or more of the components be internally relying on prod URLs?

[rsimha]

Here's a theory: src/sanitizer.js uses cdn from src/config.js, which relies on cdnUrl set in build-system/app.js. Perhaps there's a race between sanitizer.js and app.js?

[rsimha]

To help you debug, here are the amp-img image element that's causing the sanitizer error (there's one more like it further down the file), and the json data used to populate the image

[rsimha]

/cc @dvoytenko who wrote this visual test

[dreamofabear]

I don't think that's it. That cdn reference only rewrites image URLs to point to CDN.

The error complains that the following is invalid:

class="i-amphtml-element i-amphtml-notbuilt amp-notbuilt"

This is technically true. These classes are added during an AMP custom element's lifecycle (before buildCallback, obviously). However this shouldn't happen since elements that live in a <template> should not be bootstrapped before sanitization.

What browser do we run again? Some WebKit derivative right?

[rsimha]

Yeah, we use PhantomJS. See the link under https://github.com/ampproject/amphtml/blob/master/contributing/DEVELOPING.md#visual-diff-tests.

I guess there are two questions to answer:

1. Why is this failing on PhantomJS?
2. Why did a prod push affect the behavior of a locally built runtime?

[rsimha]

@choumx I've just switched the visual tests from phantomJS to headless chrome.

[rsimha]

@choumx With #13404, question 1 in #13351 (comment) is moot. I'll leave it to you to decide if question 2 is worth investigating.

[ampprojectbot]

This issue hasn't been updated in awhile. @choumx Do you have any updates?

[dreamofabear]

(2) was solved by fixing a bug in our tooling.