New issue
Advanced search Search tips

Issue 792864 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

[Missing Tests] : ExtensionSettings description too long

Project Member Reported by kkaluri@chromium.org, Dec 7 2017

Issue description

Automated tests for the below commit have been missing.Would it be possible to add test coverage to avoid regressions in future?

CL: 
===
https://chromium.googlesource.com/chromium/src.git/+/5710f8d7a584898c5e9d2caf5dc00f71d268e790

Ref Bug: 
========
https://bugs.chromium.org/p/chromium/issues/detail?id=789087
 
Cc: ljusten@chromium.org
Labels: -Pri-1 Pri-2
Owner: ----
Status: Available (was: Assigned)
We could add one but chances are low this will occur again soon. It is the first time we had this condition in over 300 policies already. Most policies are simple enough to require 2-3 sentences at most.

In either case it is certainly not on Nick to worry about that. I am changing the owner of this issue and adding some more core to the enterprise teams members that own this file.

Comment 2 by emaxx@chromium.org, Dec 15 2017

Cc: emaxx@chromium.org
Labels: Enterprise-Triaged
So if the issue was in exceeding some fixed limit on the number of characters in the policy description, should we just add a check into syntax_check_policy_template_json.py to enforce this constraint at the presubmit stage?
According to this random site on the internet:
https://books.google.de/books?id=uJ5CAwAAQBAJ&pg=PT518&lpg=PT518&dq=%22adm%22+template+explain+text+character+limit&source=bl&ots=uwYS0ujG8d&sig=tDjYdR-IoJ6Ym_aY8NsdJimuvJo&hl=en&sa=X&ved=0ahUKEwi0isnNoJPYAhUMVxQKHdR8D-wQ6AEIKjAA#v=onepage&q=%22adm%22%20template%20explain%20text%20character%20limit&f=false
the character limit for the description (aka EXPLAIN text in ADM templates) is 4096 characters. ADMX doesn't seem to have this restriction.

One option would be to just kill ADM since ADMX is supported since 2008, but Julian mentioned that ADM is still used.

Adding a check to syntax_check_policy_template_json.py might not catch everything since that check operates on the English text with macros only, not on the final translated text with macros replaced. You'd have to run it through the translation tool first to be sure. A workaround could be to add a safety margin (e.g. check >= 3072 characters).

To add on that, I think we should add this check since it's very simple and likely to catch issues.

Comment 5 by emaxx@chromium.org, Dec 22 2017

Agreed.
It's only worrying that this "4096 characters" is too vaguely defined, thinking about Unicode complications: is this talking about Unicode code points or UTF-16 code units or UTF-16 encoding bytes? I'm guessing, though, that it's likely to be UTF-16 code units, which would mean that practically this should be of the same order as the code point count in the original string (which is easy to obtain in Python).
Good point. I agree it's vague. We'd have to try that out. If it's UTF-16 bytes (i.e. raw memory size), then we'd have to cap at 1536 characters.

The description in this CL had >6500 ASCII characters (measured the 'dumb' way), so I'm guessing they mean code points.
Project Member

Comment 7 by sheriffbot@chromium.org, Dec 24

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: pastarmovj@chromium.org
Status: Started (was: Untriaged)
CL ready for review here https://chromium-review.googlesource.com/c/chromium/src/+/1391509
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 7

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

commit fe450a933106c0e4c51022bb20118147a25fa4bb
Author: Julian Pastarmov <pastarmovj@chromium.org>
Date: Mon Jan 07 13:54:07 2019

Verify the maximal description length for policies

The ADM policy templates have a limit of 4k characters for each string.
The CL adds a presubmit check with a hard error at 4k characters and a
warning at about 3k because any translation can easily exceed the
English text.

It would be nice to be able to verify that no translation exceeds the
limit too but since translations are only started on release branches
the point at which this issue will be discovered will be a rather
inconvenient point in time to fix it. Also given that the issue is only
present with the old ADM format it doesn't deserve much more than what
is done already to truncate bad translations to 4k chars and here to
try to avoid the need for that.

BUG= 792864 
TEST=presubmit passes

Change-Id: Ib1db65f416c1ac9273f0825d6a604120d0db9de8
Reviewed-on: https://chromium-review.googlesource.com/c/1391509
Commit-Queue: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620312}
[modify] https://crrev.com/fe450a933106c0e4c51022bb20118147a25fa4bb/components/policy/tools/syntax_check_policy_template_json.py

Status: Fixed (was: Started)

Sign in to add a comment