Several DistillerPageWebContentsTests failing on Android since ~ 2017-06-01T00:00:00Z due to "use strict" |
|||||||||||||||
Issue descriptionTests 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.
,
Jun 1 2017
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
,
Jun 1 2017
(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)
,
Jun 1 2017
wychen: Any idea?
,
Jun 1 2017
Looks like the returned proto is empty. The collected data on flakiness dashboard might be somewhat incomplete. https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=components_browsertests&tests=DistillerPageWebContentsTest https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=components_browsertests%20(with%20patch)&tests=DistillerPageWebContentsTest
,
Jun 1 2017
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.
,
Jun 2 2017
update: spec revert pulled from the CQ, as the CL is a spec fix for a top crasher in canary. Hunt continues.
,
Jun 2 2017
,
Jun 2 2017
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
,
Jun 2 2017
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
,
Jun 2 2017
Issue 728960 has been merged into this issue.
,
Jun 2 2017
Now failing consistently :/
,
Jun 2 2017
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/
,
Jun 2 2017
#10: what could cause options to be undefined there?
,
Jun 2 2017
Based on the intersection of the regression ranges, it appears that it's somewhere between #76913 and #76921 inclusive.
,
Jun 2 2017
b0400484bbe94dcd6d33f4cdba8fd8ce3dc0d997 still fails locally.
,
Jun 2 2017
66cf3e9f6bb5ad99f0364f80e3469e6912c27f31 (1 week old) passes. Starting third bisect.
,
Jun 2 2017
Currently the regression looks to be in the range: https://chromium.googlesource.com/chromium/src/+log/47cd98bbaa7eb5ffffee7469182446f5e2548b12..b0400484bbe94dcd6d33f4cdba8fd8ce3dc0d997
,
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
,
Jun 2 2017
dropping priority w/ the tests disabled.
,
Jun 2 2017
#18: how did you arrive at that regression range? I'm still able to reproduce the failure at 47cd98bbaa7eb5ffffee7469182446f5e2548b12
,
Jun 2 2017
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.
,
Jun 2 2017
hmm, it seemed that this is super flaky, probably not make sense to bisect, assigned it to code owner for investigating ?
,
Jun 2 2017
,
Jun 2 2017
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.
,
Jun 2 2017
(it's -> its)
,
Jun 2 2017
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.
,
Jun 3 2017
How did you get the pak files from trybots?
,
Jun 3 2017
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
,
Jun 3 2017
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/
,
Jun 4 2017
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.
,
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
,
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
,
Jun 6 2017
,
Jun 6 2017
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.
,
Jun 7 2017
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.
,
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
,
Jun 21 2017
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.
,
Jan 24 2018
,
Jan 24 2018
,
May 1 2018
The disabled tests were re-enabled in #c33. |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by jbudorick@chromium.org
, Jun 1 2017