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

Issue 654525 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 615096



Sign in to add a comment

Remove support for component extensions in telemetry

Project Member Reported by achuith@chromium.org, Oct 10 2016

Issue description

Catapult bug: https://github.com/catapult-project/catapult/issues/2919

Filing this bug because the chromeos repository doesn't understand catapult bugs.


 
Blocking: 615096
Since this is a behavior change, can you announce on telemetry-announce@ mailing list?
ok!
Digging through this, it looks like we need to make an exception for --load-component-extension on ChromeOS. I don't think there is a way to change command line flags on ChromeOS as there is on other platforms.

We have an autotestPrivate api that is available only to component extensions, and is used by ChromeOS integration tests:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/autotest_private/autotest_private_api.h

We don't want to make these APIs public, which would need to happen for them to be accessible by non-component extensions. We don't have another way of invoking these functions, which do things like lock the screen, set touchpad settings, etc.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/7df9069fc2e0aa530aec689c8e2d8b74542f9dba

commit 7df9069fc2e0aa530aec689c8e2d8b74542f9dba
Author: Achuith Bhandarkar <achuith@chromium.org>
Date: Mon Oct 10 19:35:19 2016

autotest: Remove is_component=False invocations.

is_component=False is the default, so this CL changes nothing.

BUG= chromium:654525 
TEST=trybot

Change-Id: I0eda8c87978279775c4ae1b6e31c0da4b5b10719
Reviewed-on: https://chromium-review.googlesource.com/396298
Commit-Ready: Achuith Bhandarkar <achuith@chromium.org>
Tested-by: Achuith Bhandarkar <achuith@chromium.org>
Tested-by: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Reviewed-by: Dan Shi <dshi@google.com>

[modify] https://crrev.com/7df9069fc2e0aa530aec689c8e2d8b74542f9dba/client/site_tests/desktopui_CameraApp/desktopui_CameraApp.py
[modify] https://crrev.com/7df9069fc2e0aa530aec689c8e2d8b74542f9dba/client/site_tests/logging_FeedbackReport/logging_FeedbackReport.py
[modify] https://crrev.com/7df9069fc2e0aa530aec689c8e2d8b74542f9dba/client/site_tests/accessibility_ChromeVoxSound/accessibility_ChromeVoxSound.py
[modify] https://crrev.com/7df9069fc2e0aa530aec689c8e2d8b74542f9dba/client/site_tests/network_FirewallHolePunch/network_FirewallHolePunch.py
[modify] https://crrev.com/7df9069fc2e0aa530aec689c8e2d8b74542f9dba/client/site_tests/accessibility_Sanity/accessibility_Sanity.py
[modify] https://crrev.com/7df9069fc2e0aa530aec689c8e2d8b74542f9dba/client/site_tests/documentscan_AppTestWithFakeLorgnette/documentscan_AppTestWithFakeLorgnette.py

Cc: catmulli...@chromium.org
@Achuith: Thank you for putting together these CLs! Just one observation: the particular telemetry test that relies on the --load-component-extension flag is telemetry.internal.browser.extension_unittest.ComponentExtensionTest.testComponentExtensionBasic. It lives in this file: 

https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/browser/extension_unittest.py?q=testComponentExtensionBasic&sq=package:chromium&l=167

Will you have a CL that removes support for the component extension flag in this file?
Yes, of course. 

First order of business for me is to support the whitelist for the autotest extension. Haven't gotten to it yet; hopefully soon.
Just following up: What is the status of this bug? When are you thinking of completing it? 
End of month

Comment 13 by ihf@chromium.org, Nov 12 2016

Cc: kathrelk...@chromium.org ihf@chromium.org
Labels: -Pri-2 Pri-1
Your change was not a noop and broke accessibility_Sanity
https://chromium-review.googlesource.com/#/c/396298/4/client/site_tests/accessibility_Sanity/accessibility_Sanity.py

With your change I get 
  File "/usr/local/autotest/tests/accessibility_Sanity/accessibility_Sanity.py", line 54, in run_once
    with chrome.Chrome(extension_paths=[extension_path]) as cr:
  File "/usr/local/autotest/common_lib/cros/chrome.py", line 169, in __init__
    path, self.browser_type, is_component=is_component)
  File "/usr/local/telemetry/src/third_party/catapult/telemetry/telemetry/internal/browser/extension_to_load.py", line 25, in __init__
    'Component extension %s must have a public key' % path)
MissingPublicKeyException: Component extension /usr/local/autotest/cros/a11y/a11y_ext must have a public key

Reverted the test plays on samus but fails for other reasons.
https://wmatrix.googleplex.com/unfiltered?hide_missing=True&releases=tot&tests=accessibility_Sanity&days_back=100

Shall I land the revert?
Hmm, I'm not able to make sense of your comment.

With the change that I landed, a11y_ext should be loaded as a regular extension, not a component extension. You shouldn't get the MissingPublicKeyException in that case, right?

Feel free to revert the change for this file (not the entire CL), and please open a bug (assign to me).
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/195b630c9ae66b75f17a013c7efd6e624b9ee4d8

commit 195b630c9ae66b75f17a013c7efd6e624b9ee4d8
Author: Achuith Bhandarkar <achuith@chromium.org>
Date: Wed Nov 16 22:33:58 2016

autotest: Remove use of is_component.

This is not a behavior change since is_component defaults to True.

BUG= chromium:654525 
TEST=None

Change-Id: I5ea7d1ab710e30be2075c41012851c685edb5f90
Reviewed-on: https://chromium-review.googlesource.com/412076
Commit-Ready: Achuith Bhandarkar <achuith@chromium.org>
Tested-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Wai-Hong Tam <waihong@google.com>

[modify] https://crrev.com/195b630c9ae66b75f17a013c7efd6e624b9ee4d8/server/site_tests/audiovideo_AVSync/audiovideo_AVSync.py

Project Member

Comment 16 by bugdroid1@chromium.org, Nov 18 2016

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

commit c6060ad28629511e81e3ff52eded40fbd2a9358e
Author: achuith <achuith@chromium.org>
Date: Fri Nov 18 03:05:41 2016

Whitelist for autotest extension.

The autotest extension is currently loaded as a component extension
using --load-component-extension. We would like to instead whitelist
this extension towards deprecation of the flag.

BUG= chromium:654525 

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

[modify] https://crrev.com/c6060ad28629511e81e3ff52eded40fbd2a9358e/chrome/common/extensions/api/_permission_features.json

Project Member

Comment 17 by bugdroid1@chromium.org, Nov 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/8949a8db9cd4d16f897d88241dfc0d658cf93d44

commit 8949a8db9cd4d16f897d88241dfc0d658cf93d44
Author: Achuith Bhandarkar <achuith@chromium.org>
Date: Wed Nov 16 22:28:09 2016

autotest: Remove is_component from ChromeDriver.

BUG= chromium:654525 
TEST=trybots

Change-Id: Ic1ca52cddfcd0fd4c80faf1f3c0002733e12ff8d
Reviewed-on: https://chromium-review.googlesource.com/412122
Commit-Ready: Achuith Bhandarkar <achuith@chromium.org>
Tested-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>

[modify] https://crrev.com/8949a8db9cd4d16f897d88241dfc0d658cf93d44/client/common_lib/cros/chromedriver.py

Project Member

Comment 18 by bugdroid1@chromium.org, Nov 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/11e88a9a2cdea839894a1108212e35faa7ee5c87

commit 11e88a9a2cdea839894a1108212e35faa7ee5c87
Author: Achuith Bhandarkar <achuith@chromium.org>
Date: Tue Nov 15 00:59:29 2016

autotest: Default is_component to False.

This depends on http://crrev.com/2503213003

BUG= chromium:654525 
TEST=desktopui_ScreenLocker

Change-Id: I20f5f8d262e6ab18cfd2952bbc2dc0cfd77b60e1
Reviewed-on: https://chromium-review.googlesource.com/412142
Commit-Ready: Nicolas Boichat <drinkcat@chromium.org>
Tested-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Ilja H. Friedel <ihf@chromium.org>

[modify] https://crrev.com/11e88a9a2cdea839894a1108212e35faa7ee5c87/client/common_lib/cros/chrome.py

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 25 2016

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

commit c60f3321df06eb2945441f7953e1747efadb943a
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Fri Nov 25 00:25:59 2016

Roll src/third_party/catapult/ 3276375d2..671a65473 (7 commits).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/3276375d2563..671a654736c2

$ git log 3276375d2..671a65473 --date=short --no-merges --format='%ad %ae %s'
2016-11-24 simonhatch Bisect - Fix compare_samples to read scalar values.
2016-11-24 hjd Fix getBoundingVisibleRect with offscreen elements
2016-11-24 ulan Helper functions for computing expected queueing time.
2016-11-23 achuith Autotest extension fix.
2016-11-23 benjhayden Document customizeSummaryOptions in how-to-write-metrics.
2016-11-23 fmeawad runtime_stats metric: only display the average in the dashboard
2016-11-23 simonhatch [bisect] - Clearly state bisect type in output.

BUG= 668536 , 664515 ,625701, 654525 , 667813 

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=catapult-sheriff@chromium.org

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

[modify] https://crrev.com/c60f3321df06eb2945441f7953e1747efadb943a/DEPS

Project Member

Comment 20 by bugdroid1@chromium.org, Dec 1 2016

Project Member

Comment 21 by bugdroid1@chromium.org, Dec 2 2016

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

commit e61ba97f267c03d3bc8e991d09e5b9592850933a
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Fri Dec 02 02:58:05 2016

Roll src/third_party/catapult/ f044f07b3..36566ddb1 (2 commits).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/f044f07b34a0..36566ddb1d53

$ git log f044f07b3..36566ddb1 --date=short --no-merges --format='%ad %ae %s'
2016-12-01 achuith Remove support for component extensions.
2016-12-01 sheu Dynamically scale CPU usage track interval

BUG= 654525 

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=catapult-sheriff@chromium.org

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

[modify] https://crrev.com/e61ba97f267c03d3bc8e991d09e5b9592850933a/DEPS

Status: Fixed (was: Assigned)
Should be fixed. I'd give it a few days to make sure nothing gets reverted.

Comment 23 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 24 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 25 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 27 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment