New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 728655 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 729689



Sign in to add a comment

Several DistillerPageWebContentsTests failing on Android since ~ 2017-06-01T00:00:00Z due to "use strict"

Project Member Reported by jbudorick@chromium.org, Jun 1 2017

Issue description

Tests failing:

DistillerPageWebContentsTest.UsingCurrentWebContentsNoMainFrameObserver
DistillerPageWebContentsTest.HandlesRelativeVideos
DistillerPageWebContentsTest.BasicDistillationWorks
DistillerPageWebContentsTest.UsingCurrentWebContentsWrongUrl
DistillerPageWebContentsTest.HandlesRelativeImages
DistillerPageWebContentsTest.VisibilityDetection
DistillerPageWebContentsTest.MarkupInfo
DistillerPageWebContentsTest.HandlesRelativeLinks

Seeing this on:

M64 bot:
 - https://uberchromegw.corp.google.com/i/chromium.android/builders/Marshmallow%2064%20bit%20Tester/builds/12664

N5X trybot:
 - https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/191042
 - https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/191113
 - https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/191166

Looking for a culprit.
 
Cc: mdjones@chromium.org nyquist@chromium.org bengr@chromium.org szager@chromium.org wychen@chromium.org
The first occurrence linked in #1 came from https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/190739. The got_revision in that CL is crrev.com/948a6f1c5a71a794d102c28367ee64c5c81b2f3e.

+szager: any idea if your change could be responsible for this?

also, +cc dom_distiller owners
(note that this is happening frequently on the N5X trybot; the examples I linked above are not an exhaustive list: https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel?numbuilds=200)
wychen: Any idea?
Summary: Several DistillerPageWebContentsTests failing (was: Several DistillerPageWebContentsTests failing on M)
this is also failing at a high rate on the K trybot: https://chromium-swarm.appspot.com/tasklist?c=name&c=state&c=created_ts&c=user&c=bot&et=1496360100000&f=name%3Acomponents_browsertests&f=buildername%3Alinux_android_rel_ng&f=state%3ACOMPLETED_FAILURE&l=50&n=true&q=state%3A&s=created_ts%3Adesc&st=1496187300000

In the absence of other plausible candidates or a response from szager, I'm going to spec revert http://crrev.com/948a6f1c5a71a794d102c28367ee64c5c81b2f3e.
update: spec revert pulled from the CQ, as the CL is a spec fix for a top crasher in canary. Hunt continues.
Summary: Several DistillerPageWebContentsTests failing on Android since ~ 2017-06-01T00:00:00Z (was: Several DistillerPageWebContentsTests failing)
Cc: michaelbai@chromium.org tobiasjs@chromium.org
FWIW -- I kicked a few tryjobs on the spec revert CL, one of which still exhibited the failure, so it definitely isn't szager's CL. No current suspect, though.

+cc tomorrow's android sheriffs: fyi
In the failed cases, they have one more message:

[INFO:CONSOLE(215)] "Uncaught ReferenceError: options is not defined", source:  (215)

I think the variable |options| might be this one:
https://cs.chromium.org/chromium/src/components/dom_distiller/core/javascript/domdistiller.js?type=cs&q=options+f:domdistiller.js&l=7
 Issue 728960  has been merged into this issue.
Labels: -Pri-1 Pri-0
Now failing consistently :/
Tried to run manual bisect, so far what I can tell is that these tests are already failing locally ~1 day, extending scope...

In the meantime, CL to disable these tests in CQ: https://codereview.chromium.org/2916213003/
#10: what could cause options to be undefined there?
Based on the intersection of the regression ranges, it appears that it's somewhere between #76913 and #76921 inclusive.
b0400484bbe94dcd6d33f4cdba8fd8ce3dc0d997 still fails locally.
66cf3e9f6bb5ad99f0364f80e3469e6912c27f31 (1 week old) passes. Starting third bisect.
Project Member

Comment 19 by bugdroid1@chromium.org, Jun 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea4ad773e8a3b6fefb566abc3aa396eb2f4058fe

commit ea4ad773e8a3b6fefb566abc3aa396eb2f4058fe
Author: engedy <engedy@chromium.org>
Date: Fri Jun 02 16:10:08 2017

Disable failing DistillerPageWebContentsTests tests on Android.

BUG= 728960 ,728655
TBR=nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2916213003
Cr-Commit-Position: refs/heads/master@{#476682}

[modify] https://crrev.com/ea4ad773e8a3b6fefb566abc3aa396eb2f4058fe/components/dom_distiller/content/browser/distiller_page_web_contents_browsertest.cc

Cc: jbudorick@chromium.org engedy@chromium.org
Labels: -Pri-0 Pri-1
Owner: ----
Status: Available (was: Assigned)
dropping priority w/ the tests disabled.
#18: how did you arrive at that regression range? I'm still able to reproduce the failure at 47cd98bbaa7eb5ffffee7469182446f5e2548b12
So far it was quite reliably passing/failing for me at any given revision. Not anymore. Now it passes for me at b0400484bbe94dcd6d33f4cdba8fd8ce3dc0d9. :'(

I guess the best is if you ignore all ranges I have mentioned above. Sorry.
hmm, it seemed that this is super flaky, probably not make sense to bisect, assigned it to code owner for investigating ?
Owner: wychen@chromium.org
This may be a build flake of some kind (yay) involving components_tests_resources.pak. On comparing a pak file from a passing test against one from a failing test, the failing test has 'use strict' at the start of many (all?) of it's scripts.
(it's -> its)
That could explain why I was seeing either all tests pass or all tests fail from a certain build, as well as why I got different results when I built a certain revision again.
How did you get the pak files from trybots?
Ah. The pak files are available in isolated inputs.

In the failed cases, closure compiler behaves as if "language_out=ECMASCRIPT5_STRICT" is given.

Could the culprit be the roll of closure compiler?
https://codereview.chromium.org/2918683002
Status: Started (was: Available)
The culprit is indeed closure compiler. I'm suspecting this:
https://github.com/google/closure-compiler/commit/fe30d6f16a05bc62fb05d4ff4c60cc9979d7f428

See the trybot results in PS1 and PS2 here:
https://codereview.chromium.org/2921143002/
Components: Build
The key change was actually this:

https://github.com/google/closure-compiler/commit/5279eb46b805f4a8c8d066a0b6d37f64653ada60

This made ES6 default to 'use strict'.

In our use case, we want to use ES6 syntax, but don't want 'use strict', so https://codereview.chromium.org/2921143002/ is still a good solution.

We might want to eventually turn on 'use strict' in our JS code, but I'm not sure about the test coverage of them, and it might be more trouble than it's worth. If we do have enough test coverage, more optimization than whitespace might be a better use of effort.
Project Member

Comment 32 by bugdroid1@chromium.org, Jun 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7deaa85c11854d2066e061ed93665ea920f4cbf

commit e7deaa85c11854d2066e061ed93665ea920f4cbf
Author: wychen <wychen@chromium.org>
Date: Tue Jun 06 03:25:42 2017

Fix underspecified closure dependency of js_minify.py

third_party/closure_compiler/compiler/compiler.jar was not in the
dependency of grit JS minifier. This caused flaky building behavior
and made bisecting for crbug.com/728655 difficult.

BUG=728655

Review-Url: https://codereview.chromium.org/2924653002
Cr-Commit-Position: refs/heads/master@{#477176}

[modify] https://crrev.com/e7deaa85c11854d2066e061ed93665ea920f4cbf/tools/grit/grit_rule.gni

Project Member

Comment 33 by bugdroid1@chromium.org, Jun 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14fb5d1f03c1d705a5c0e1d33904df5da7153221

commit 14fb5d1f03c1d705a5c0e1d33904df5da7153221
Author: wychen <wychen@chromium.org>
Date: Tue Jun 06 04:18:58 2017

Reverse accidental enabling of 'use strict' in closure compiler

In grit, JavaScript files are minified using closure compiler. The output
is in ES6 without "use strict", until a roll in
https://codereview.chromium.org/2918683002, which adds "use strict" by default
for ES6 (now called ES2015).

Having "use strict" caused failures in DOM distiller and probably other things,
so we need to fix them first.

BUG=728655, 729689
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2921143002
Cr-Commit-Position: refs/heads/master@{#477192}

[modify] https://crrev.com/14fb5d1f03c1d705a5c0e1d33904df5da7153221/components/dom_distiller/content/browser/distiller_page_web_contents_browsertest.cc
[modify] https://crrev.com/14fb5d1f03c1d705a5c0e1d33904df5da7153221/third_party/closure_compiler/closure_args.gni

Blocking: 729689
The DOM distiller JavaScript code generated by GWT cannot work in strict mode. We are using GWT 2.7.0, which was released on 2014-11-20. Might be time to update to latest GWT, or migrate to J2CL.
Latest GWT 2.8.1 seems to generate code working in strict mode, but the built-in closure compiler bundle is gone. I haven't been able to use closure compiler as a postprocessor to generate working code that is not larger than 2.7.0 output.
Project Member

Comment 37 by bugdroid1@chromium.org, Jun 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b24059c04caebfc2542f765a6064ccc5a6d1b27d

commit b24059c04caebfc2542f765a6064ccc5a6d1b27d
Author: wychen <wychen@chromium.org>
Date: Wed Jun 07 01:41:51 2017

Remove "named arguments" in domdistiller.js

Unlike Python, there's no named arguments in JavaScript.
Now this works in strict mode.

The transpiled JavaScript in
third_party/dom_distiller_js/dist/js/domdistiller.js still doesn't work
in strict mode.

BUG=728655

Review-Url: https://codereview.chromium.org/2924983002
Cr-Commit-Position: refs/heads/master@{#477508}

[modify] https://crrev.com/b24059c04caebfc2542f765a6064ccc5a6d1b27d/components/dom_distiller/core/javascript/domdistiller.js

Labels: -Pri-1 Pri-2
Summary: Several DistillerPageWebContentsTests failing on Android since ~ 2017-06-01T00:00:00Z due to "use strict" (was: Several DistillerPageWebContentsTests failing on Android since ~ 2017-06-01T00:00:00Z)
This bug tracks the effort to make generated JS code of DOM distiller run with "use strict".
Lower the priority since "use strict" is removed.
Components: Tests>Disabled
Labels: Test-Disabled
Components: -Tests>Disabled
Labels: -Test-Disabled
Status: Assigned (was: Started)
The disabled tests were re-enabled in #c33.

Sign in to add a comment