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

Issue 786061 link

Starred by 12 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 787586



Sign in to add a comment

Using "ExtensionAllowedTypes" = "hosted_app" results in users unable to launch bookmarks from shelf

Project Member Reported by newcomer@chromium.org, Nov 16 2017

Issue description

Reported in M62, still broken in ToT.
Reported here 784770 

What steps will reproduce the problem?
(1) Manually set up an enterprise policy with "ExtensionAllowedTypes" = "hosted_app"
(2) Try to open a bookmark which was attached to shelf (via chrome settings -> more tools -> Add to shelf
(3) Try to open the bookmark from the shelf

What is the expected result?
The bookmark opens in chrome

What happens instead?
The pinned shelf icon flashes, but does nothing.

 
Cc: vadimt@chromium.org newcomer@chromium.org
 Issue 784770  has been merged into this issue.
Cc: bartfab@chromium.org atwilson@chromium.org
Labels: -Pri-3 OS-Chrome Pri-1
Owner: newcomer@chromium.org
Status: Assigned (was: Untriaged)
+Enterprise folks since this is enterprise policy.
It's blocking schools from operating correctly so I'm raising priority to P1, thinking even of P0.

 Issue 785331  has been merged into this issue.
Cc: msw@chromium.org khmel@chromium.org
+msw for shelf, and +khmel.

Comment 5 by msw@chromium.org, Nov 16 2017

Cc: michae...@chromium.org
I don't know how to setup an enterprise policy locally :-/
I would have suggested trying --ash-disable-shelf-model-synchronization, but that feature is off-by-default on M62.
Maybe check that AppShortcutLauncherItemController::ItemSelected is calling ChromeLauncherController::LaunchApp?
I have set up a device with a test policy.

I enabled all extension types using "ExtensionAllowedTypes", which includes these values :
"extension", "theme", "user_script", "hosted_app", "legacy_packaged_app", "platform_app"

And the result is that bookmarked apps on the shelf don't launch, and launching a bookmarked app from the launcher results in a crash.

I'm still digging, and I'll try logging the functions you mentioned.
Bookmarked app = a bookmark which is pinned to the shelf via chrome -> settings -> more tools -> "Add to shelf..."

Comment 8 Deleted

After bookmarking a random google search and adding it to the shelf, and ensuring that device policy enables all extensions, the extensions are still being disabled.

[8603:8603:1116/151819.850803:WARNING:extension_error_reporter.cc(77)] Extension error: Enterprise DeviceAttributes Test Extension (extension ID "ilnpadgckeacioehlommkaafedibdeob") is blocked by the administrator. 
[8603:8603:1116/151820.045815:WARNING:extension_error_reporter.cc(77)] Extension error: Chrome Sign Builder (extension ID "odjaaghiehpobimgdjjfofmablbaleem") is blocked by the administrator. 
[8603:8603:1116/151820.126012:WARNING:extension_error_reporter.cc(77)] Extension error: Sign-in Screen Test App (extension ID "bjaiihebfngildkcjkjckolinodhliff") is blocked by the administrator. 
[8603:8603:1116/151820.235104:WARNING:extension_error_reporter.cc(77)] Extension error: TPM Update Status (extension ID "nkeelpdlenncgfhkanbhagmhpaebebep") is blocked by the administrator. 

Cc: dhadd...@chromium.org
+test team, can you help bisect to help find when the regression was introduced?

Comment 11 by ka...@chromium.org, Nov 17 2017

Cc: kathrelk...@chromium.org krishna...@chromium.org
It seems like newly installed web-shortcut extensions are being disabled with the extensions::disable_reason::DISABLE-REASON-LAST. 
Cc: ortuno@chromium.org dskaram@chromium.org
Components: -UI>Shell>Shelf
Owner: benwells@chromium.org
Adding Benwells@ and ortuno@ after talking offline.

Repro Steps:
1. Image a device with the newest M62 test image.
2. Follow the Test DMS https://docs.google.com/document/d/1HiSuCNESc6tOQxx2QdmYATy4mT_e1zUaocmqUvJyWeU/edit#

3. Enable policy "ExtensionAllowedTypes" with values "extension", "theme", "user_script", "hosted_app", "legacy_packaged_app", "platform_app"
4. Try to launch a web shortcut from the shelf (add one via chrome browser -> hamburger menu -> More tools -> "Add to shelf..."

Logs revealed that installing a web-shortcut extension with ExtensionService::OnExtensionInstalled would get extensions::disable_reason::DISABLE_REASON_LAST upon install which seems broken.
Components: UI>Shell>Shelf
Cc: jamescook@chromium.org rdevlin....@chromium.org
Wait, the extensions are actually becoming *disabled* when launched from the shelf?

What about launching bookmarked apps from chrome://apps instead of using the shelf?
Cc: r...@chromium.org
Labels: ReleaseBlock-Stable
I can't see how this isn't a relesae blocker.
Cc: calamity@chromium.org
calamity, any chance this is related to recent PWA work?

hrm, I can't repro this with a managedchrome.com account on ToT Chrome OS-on-Linux.

BTW, when I set the ExtensionAllowedTypes to the values in #13 (all the checkboxes checked), then ExtensionAllowedTypes doesn't show up in chrome://policy. Do you have other policies set?
Cc: karandeepb@chromium.org
"Extensions: Change how ManagementPolicy::UserMayLoad is handled" - crrev.com/c/611662 looks possibly relevant. Still can't repro though.

The DISABLE_BLOCKED_BY_POLICY disable_reason was added, so it took on the new value of DISABLE_REASON_LAST. This is persisted in extension prefs. But that would only matter if you're using prefs from a newer version on an older build, right?

Another wild theory: are bookmark apps even supposed to be blockable? (Why would "Add to shelf" show up in the Chrome menu then?) If not, maybe some special case for bookmark apps no longer works due to extensions being disabled by policy?
As per https://bugs.chromium.org/p/chromium/issues/detail?id=785331#c3, this happens when ExtensionInstallBlacklist is set to block everything.  newcomer@: Did you have this policy set as well while reproducing this?

If that's the case, as c#19 points out crrev.com/c/611662 might be relevant and possibly some special case for bookmark apps no longer works.

Would it be possible to repro this on a Linux workstation using 'target_os="chromeos"? I don't have access to a ChromeOS device.
chrome://apps is disabled on ChromeOS

I can't get the behavior to repro on ToT with just "hosted_apps" enabled in policy. If I remove it from the allowed types in policy, then it takes the behavior described here (which is more or less expected since hosted apps are disabled and bookmark apps are hosted apps).

I don't think recent PWA work would have affected this. 2 milestones ago is before most of the overhauling work.

FYI for repro, I'm using the magic ChromeOS policy file for linux chromeos testing that you can write to at /etc/chromium/policies/managed/test_policy.json with a value of

{
  "ExtensionAllowedTypes": [
    "extension",
    "theme",
    "user_script",
    "legacy_packaged_app",
    "platform_app"
  ]
}

If we want a special case for bookmark apps and PWAs here, we should add extra allowed types so that it's explicit.
calamity@: The CrOS bookmark apps being talked about in this bug are the same as the bookmark apps on on other Desktop platforms which appear on chrome://apps, correct?
Yeah, that's right. If this turns out to be because the blacklist is on, I'm not sure what the right way forward is. The way forward will depend on @newcomer's response, since I can't get this to repro with only the hosted_app allow policy enabled.
So I also wasn't able to repro this on Linux with ExtensionAllowedTypes set to hosted_app. newcomer@: what do you see on chrome://policy?

However, I was able to repro this with ExtensionInstallBlacklist policy set to block all extensions ( crbug.com/785331 ) (Which is probably WAI since bookmark apps are a kind of extension). Also I investigated and this behavior changed after crrev.com/c/611662 which made the ManagementPolicy checks more consistent. Here's what happended for bookmark apps before crrev.com/c/611662 with ExtensionInstallBlacklist set to "*".

On launching the browser,
- InstalledLoader::Load would fail the ManagementPolicy::UserMayLoad check causing the bookmark app to not be enabled.
- But later ExtensionSyncService::ApplyBookmarkAppSyncData would lead to a call to CrxInstaller::InstallWebApp causing the bookmark app to be installed/enabled. This code path didn't check ManagementPolicy::UserMayLoad, which was a bug.

After crrev.com/c/611662, the bookmark app would failed the ManagementPolicy check in InstalledLoader and get disabled (earlier it was not loaded). Hence now during the  ExtensionSyncService::ApplySyncData code path, the extension would be recognised as being disabled and we won't try to enable it again.

So two questions:
  - Do we want to special case bookmark apps so that they can't be blacklisted (with ExtensionInstalledBlacklist/ExtensionAllowedTypes and possibly other policies)?
  - Are bookmark apps not working with ExtensionAllowedTypes set to "hosted_app". This would be a bug.

Also I haven't yet looked into the disable reason being extensions::disable_reason::DISABLE_REASON_LAST which seems wrong.
So, how do "bookmark apps" differ from regular web pages? I'm presuming that bookmarks apps are bookmarks, and so should be managed with bookmark policies (i.e. EditBookmarksEnabled, URLBlacklist) not with extension policy.

My suggestion is: Yes, we want to special case "bookmark apps" so they can't be blacklisted by extension policy. I don't think we'd treat them as hosted apps, because unlike hosted apps, you don't pre-grant permissions to bookmarks.

Again, apologies if "bookmark apps" is a real thing with pre-granted permission, etc, rather than just regular ol' bookmarks pinned to the shelf with maybe some extra window dressing and I didn't get the memo.
I agree with #25 that enterprise policy for apps/extensions probably shouldn't apply to bookmark apps.

But bookmark apps are not bookmarks, they're actually apps. Indeed, they don't grant permissions; they do nothing but afford an icon in the shelf and launcher.

Whether we should use the policy for hosted apps (bookmark apps are a permissionless subset of hosted apps), or bookmarks, or no policy at all depends on what administrators will want to enforce:

* Are admins interested in restricting the permissions/capabilities users can grant to apps/websites? Allow restricting hosted apps, but don't apply that policy to bookmark apps.

* Are admins interested in preventing users from adding icons to the launcher or the shelf? Allow restricting apps in a way that covers bookmark apps.

The second point there is that bookmark apps look and feel sort of like apps (they have icons and are uninstallable); will admins want to restrict that UX for some reason? (Is there another policy that already does this?)


Technobabble follows:

Adding a website to the shelf creates a bookmark app. This is a skeleton app that Chrome generates on-demand in response to that action. The app is created in such a way that launching it does nothing but open the given URL.

The app has an icon in the launcher (or on chrome://apps outside Chrome OS) and can be pinned to the shelf, but has zero other "app" properties from a UX perspective AFAIK.

The fact that we implement it as an app is a technical/historical detail, I think -- it's a way to get the icon into the launcher and the shelf.

More clarifications available at https://chromium.googlesource.com/chromium/src/+/master/extensions/docs/extension_and_app_types.md.
Yeah, I looked at it a bit more deeply after writing the comment, and "bookmark apps" indeed are not bookmarks (cue Dr. Nick). So ignore everything I wrote before except for the piece that "bookmark apps should not be controlled by extension policy" :)

We've had no customer requests to block pinning web pages to the shelf, so for now let's just leave this functionality unmanaged.
It's true that right now Bookmark Apps have no other "app" properties but that is changing with Desktop PWAs. Desktop PWAs will be able to capture links, just like Android apps do, and have a nicer window frame than regular Hosted Apps. It's quite possible that Desktop PWAs will be auto granted permissions in the future, the same way we auto grant some permissions to PWAs on mobile e.g. extra storage, keyboard lock, etc. This is up in the air, but I wouldn't rule it out.
We're talking about bookmark apps, though, not PWAs -- or are PWAs implemented as bookmark apps?
Correct. Desktop PWAs are implemented as Bookmark Apps.
If pinning a PWA to the shelf provides extra capabilities for the app beyond a nicer window frame, then we may need to enable some management. Otherwise, if their capabilities are identical regardless of whether they are pinned to the shelf or opened in a browser tab, I don't see any reason why we'd add extra management (if admins don't want users to grant permissions to various origins, then they should control this via content settings the same as other web pages).
Project Member

Comment 32 by sheriffbot@chromium.org, Nov 20 2017

This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: M-63
Re #18: 
 - I would suggest repro-ing it on a device. 
 - If you're setting ExtensionAllowedTypes and it isn't showing up in chrome://policy, you are probably doing something incorrect, right? This should show up in chrome://policy.

Re #20:
 - ExtensionInstallBlacklist was not set on the policy that I used to repro the bug, only the policy set in the repro steps.
 - I would recommend reproing this on a device after seeing others having trouble reproing this on linux.

Re #21: 
 - I'm not sure how valid this is because you're repro-ing on linux chromeos.
 - I reproed this when I enabled all types of apps, but this was done on device not on the emulator.

re #24:
 - When I enabled all of the extensions, I saw all of them in chrome://policy.
 - I didn't have "ExtensionInstallBlacklist" set, I'm not sure how that policy is related.


I'm a bit concerned that some of the discussion is based on reproing using a different policy/on the emulator. I don't have much knowledge on the policy settings, but are we sure we are barking up the right tree? 

Those that have been un-able to repro on-device: 
 - Was it because it's just easier to attempt to repro on the emulator, or that the on-device repro steps didnt work for you?

Setting the proper milestone for the RBS.

Owner: karandeepb@chromium.org
>>I'm a bit concerned that some of the discussion is based on reproing using a different policy/on the emulator. I don't have much knowledge on the policy settings, but are we sure we are barking up the right tree? 

In any case, that's also something we want to solve since the behavior of bookmark apps changed after crrev.com/c/611662 and there have been bug reports for the same. ( crbug.com/785331 )

I don't have access to a Chrome OS Device currently. In the meanwhile, I'll work on a patch to special case bookmark apps for ExtensionInstallBlacklist and ExtensionAllowedTypes. Later will see if if helps the repro case that newcomer@ is experiencing (When ExtensionAllowedTypes is set to allow hosted apps).
#33: It's hard to debug and dive in without a repro on my dev machine. Also, although I have a CrOS device, I am finding the instructions in that doc hard to follow. (e.g I can't open VT2 on my ChromeOS device, looked it up, needed to set to Dev mode. Now I'm stuck with the make_dev_ssd.sh step, which just fails?? Oh, I need debugging features enabled. I enabled it but didn't set a root password and it said it would be the default, but I don't know what that is?! AGGHH. Give up.).

I am surprised that the policy is executing differently on the emulator vs a device because once the policy is picked up by CrOS, all the logic for what turns on/off should be the same.

We are trying to distill possible causes for your behavior, which is why the ExtensionInstallBlacklist was brought up. If ExtensionInstallBlacklist is set to *, your behavior reproduces (on the emulator), so we were wondering if that was the cause. Since it's not, we investigate onwards!

Status: Started (was: Assigned)
#35: the default password is test0000 ;-)

The behavior *should* be the same on Chrome OS-on-Linux with the right policy setup (using managedchrome.com, a local policy server, or writing to /etc/chromium/policies/managed/test_policy.json). It's not an "emulator" -- it's the same chrome you get on a device, just with some system services stubbed out. Policy should work mostly the same AIUI.
Labels: M-62
Per #24, seems we have a way forward for at least some of the issues here.

So we have several different reports:

* Setting ExtensionAllowedTypes to ['hosted_app'] prevents bookmark apps from
  opening (#1)

  This is weird because bookmark apps *are* of type hosted_app.

  But this behavior could not be verified by #21 or #24.

* Enabling all extension types in ExtensionAllowedTypes prevents bookmark apps
  from opening from the shelf, causes launching bookmark apps from the launcher
  to crash Chrome (!) (#6), and sets the extension's disable reason to
  DISABLE_REASON_LAST (!) (#13)

  The policy is used in ExtensionManagement::IsAllowedManifestType:
  https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_management.cc?type=cs&q=%22allowed_types%22+file:extension_management&l=159

  But again, bookmark apps are TYPE_HOSTED_APP, so all indicators are that this
  policy should not prevent loading bookmark extensions:
  https://cs.chromium.org/chromium/src/chrome/browser/extensions/standard_management_policy_provider.cc?type=cs&l=99

  I suspect the underlying issue is elsewhere. Need to get a stack trace from
  launching an item from the shelf and launcher.

* Setting ExtensionInstallBlacklist to ['*'] prevents bookmark apps from opening
  ( issue 785331 ). Reported in 62 and 63.

  This makes sense given that bookmark apps are extensions. #24 and #34 suggest
  a fix.


Note: ExtensionInstallBlacklist and ExtensionAllowedTypes are supported on
linux/mac/win, so it's possible this issue repros on non-CrOS chrome as well.


BTW, the difficulty of troubleshooting policy stuff is tracked in issue 752709.

#35: +1 on the difficulty of following the policy troubleshooting instructions!

Glad that's being tracked somewhere.
#38:

Yeah I was able to repro it in non-CrOS (Linux). 

Have a fix for whitelisting bookmark apps for ExtensionAllowedTypes, ExtensionInstallBlacklist and ExtensionSettings policies under review - https://chromium-review.googlesource.com/c/chromium/src/+/780205.

Also for ChromeOS folks: App icons which correspond to disabled apps in CrOS launcher/shell should either be shown disabled (believe chrome://apps shows them as greyed out) or they should not be shown. The fact that there is no visual indication that the app is disabled and nothing happens when you click a disabled app seems like a bug.

Blocking: 787586
#40:
We have logic for that here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_app_icon.cc?q=chrome_app_icon&sq=package:chromium&l=116

I'll file a speculative bug (787586) until we are sure they are not acting properly after your patch. Blocking on this one.
Any updates? M63 goes stable in < 2 weeks and this is marked as a blocker.
The change https://chromium-review.googlesource.com/c/chromium/src/+/780205 is still under review. I am away till Monday, so would not probably be able to get to it till then.
Project Member

Comment 45 by bugdroid1@chromium.org, Nov 24 2017

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

commit 96b044cff4cbc1b6bd30bfa387b3f55d07079814
Author: Christopher Lam <calamity@chromium.org>
Date: Fri Nov 24 03:29:46 2017

Extensions: Whitelist bookmark apps in StandardManagementPolicyProvider.

r496521 made the extension management policy checks more consistent. This had a
side effect of disabling bookmark apps when the appropriate
ExtensionInstallBlacklist/ExtensionAllowedTypes/ExtensionSettings policies were
set. This CL whitelists bookmark apps in
StandardManagementPolicyProvider::UserMayLoad. Hence the bookmark apps should no
longer get disabled by the ExtensionInstallBlacklist, ExtensionAllowedTypes and
ExtensionSettings policies.

Based on https://chromium-review.googlesource.com/#/c/780205/

TBR=rdevlin.cronin@chromium.org
BUG= 786061 
TEST=On Linux, navigate to google.com. Use chrome -> settings -> more tools ->
"Add to Desktop..." to create a desktop shortcut for google.com. Change
ExtensionInstallBlacklist to block all extensions ("*"). Ensure the same from

chrome: //policy. Click on the google.com shortcut created earlier (from
chrome: //apps or Desktop) and ensure that it works correctly.
Change-Id: Ic2ac1ddd2dfef6c5442eb7b0d366eaf66443c51d
Reviewed-on: https://chromium-review.googlesource.com/787011
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519052}
[modify] https://crrev.com/96b044cff4cbc1b6bd30bfa387b3f55d07079814/chrome/browser/extensions/standard_management_policy_provider.cc
[modify] https://crrev.com/96b044cff4cbc1b6bd30bfa387b3f55d07079814/chrome/browser/policy/policy_browsertest.cc

Labels: Merge-Request-63
Status: Fixed (was: Started)
Project Member

Comment 47 by sheriffbot@chromium.org, Nov 24 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: We are only 10 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
 Issue 775359  has been merged into this issue.
Waiting for this to be validated on tot before merging into M63
Status: Verified (was: Fixed)
I was able to verify the fix on ToT on ChromeOs-on-Linux. Bookmark apps should now be exempted from extension blacklist policy checks.
Status: Fixed (was: Verified)
Talked to gkihumba@: Still needs TE verification. 
Labels: -Merge-Review-63 Merge-Approved-63
Thanks everyone!
Project Member

Comment 54 by bugdroid1@chromium.org, Nov 28 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc4a5c30819b653750594eda95f334dab490ad0e

commit fc4a5c30819b653750594eda95f334dab490ad0e
Author: Christopher Lam <calamity@chromium.org>
Date: Tue Nov 28 23:55:11 2017

Merge M63: Extensions: Whitelist bookmark apps in StandardManagementPolicyProvider.

Original description:
>r496521 made the extension management policy checks more consistent. This had a
>side effect of disabling bookmark apps when the appropriate
>ExtensionInstallBlacklist/ExtensionAllowedTypes/ExtensionSettings policies were
>set. This CL whitelists bookmark apps in
>StandardManagementPolicyProvider::UserMayLoad. Hence the bookmark apps should no
>longer get disabled by the ExtensionInstallBlacklist, ExtensionAllowedTypes and
>ExtensionSettings policies.
>
>Based on https://chromium-review.googlesource.com/#/c/780205/
>
>TBR=rdevlin.cronin@chromium.org
>BUG= 786061 
>TEST=On Linux, navigate to google.com. Use chrome -> settings -> more tools ->
>"Add to Desktop..." to create a desktop shortcut for google.com. Change
>ExtensionInstallBlacklist to block all extensions ("*"). Ensure the same from
>
>chrome: //policy. Click on the google.com shortcut created earlier (from
>chrome: //apps or Desktop) and ensure that it works correctly.
>Change-Id: Ic2ac1ddd2dfef6c5442eb7b0d366eaf66443c51d
>Reviewed-on: https://chromium-review.googlesource.com/787011
>Reviewed-by: calamity <calamity@chromium.org>
>Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
>Commit-Queue: calamity <calamity@chromium.org>
>Cr-Commit-Position: refs/heads/master@{#519052}

TBR=calamity@chromium.org

Change-Id: Ic2ac1ddd2dfef6c5442eb7b0d366eaf66443c51d
Reviewed-on: https://chromium-review.googlesource.com/794880
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#593}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/fc4a5c30819b653750594eda95f334dab490ad0e/chrome/browser/extensions/standard_management_policy_provider.cc
[modify] https://crrev.com/fc4a5c30819b653750594eda95f334dab490ad0e/chrome/browser/policy/policy_browsertest.cc

Status: Verified (was: Fixed)
Enterprise test team was able to repro this in M62 only with ExtensionInstallBlacklist set to "*" and ExtensionAllowedTypes set to "hosted_app", the pinned shelf icon flashes, but does nothing.

Verified fixed in M63. The bookmark opens from shelf if ExtensionInstallBlacklist set to "*".

Chrome: 63.0.3239.73
Chrome OS: 10032.60.0
Hi team,

Do we have any updates in this matter?

Thanks in advance.
#55 says it's fixed in M63. What other updates do need?

Sign in to add a comment