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

Issue 675506 link

Starred by 2 users
Status: Fixed
Owner:
adanilo@chromium.org
Closed: Nov 5
Cc:
█ mtomasz@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

file_manager/file_manager/common/js/file_type.js uses escaped string for RegExp consturctor.

Project Member Reported by yosin@chromium.org, Dec 19 2016 
"file_types.js" uses backslash notation in string literal for RegExp constructor, but it doesn't mean escaped RegExp syntax characters. They should be written as "\\." or "[.]" and so on.


ui/file_manager/file_manager/common/js/file_type.js(248) LEXER_ERROR_STRING_LITERAL_BACKSLASH
RegExp('/application\/(msword|vnd\.openxmlformats\-' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(248) LEXER_ERROR_STRING_LITERAL_BACKSLASH
ication\/(msword|vnd\.openxmlformats\-' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(248) LEXER_ERROR_STRING_LITERAL_BACKSLASH
|vnd\.openxmlformats\-' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(249) LEXER_ERROR_STRING_LITERAL_BACKSLASH
     'officedocument\./wordprocessingml\.document)/i')
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(249) LEXER_ERROR_STRING_LITERAL_BACKSLASH
t\./wordprocessingml\.document)/i')
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(254) LEXER_ERROR_STRING_LITERAL_BACKSLASH
RegExp('/application\/(vnd\.ms-powerpoint|\.' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(254) LEXER_ERROR_STRING_LITERAL_BACKSLASH
('/application\/(vnd\.ms-powerpoint|\.' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(254) LEXER_ERROR_STRING_LITERAL_BACKSLASH
(vnd\.ms-powerpoint|\.' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(255) LEXER_ERROR_STRING_LITERAL_BACKSLASH
     'openxmlformats\-/officedocument\.wordprocessingml\.presentation)/i')
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(255) LEXER_ERROR_STRING_LITERAL_BACKSLASH
ats\-/officedocument\.wordprocessingml\.presentation)/i')
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(255) LEXER_ERROR_STRING_LITERAL_BACKSLASH
nt\.wordprocessingml\.presentation)/i')
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(260) LEXER_ERROR_STRING_LITERAL_BACKSLASH
RegExp('/application\/(vnd\.ms-excel|\.openxmlformats\-/' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(260) LEXER_ERROR_STRING_LITERAL_BACKSLASH
('/application\/(vnd\.ms-excel|\.openxmlformats\-/' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(260) LEXER_ERROR_STRING_LITERAL_BACKSLASH
ion\/(vnd\.ms-excel|\.openxmlformats\-/' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(260) LEXER_ERROR_STRING_LITERAL_BACKSLASH
cel|\.openxmlformats\-/' +
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(261) LEXER_ERROR_STRING_LITERAL_BACKSLASH
     'officedocument\.wordprocessingml\.sheet)/i')
                    ~~
ui/file_manager/file_manager/common/js/file_type.js(261) LEXER_ERROR_STRING_LITERAL_BACKSLASH
nt\.wordprocessingml\.sheet)/i')
                    ~~

Comment 1 by mtomasz@chromium.org, Dec 8 2017

Labels: from-mtomasz

Comment 2 by mtomasz@chromium.org, Dec 8 2017

Cc: mtomasz@chromium.org
Owner: fukino@chromium.org

Comment 3 by sashab@chromium.org, Feb 24 2018

Labels: CrOS-FilesApp-CodeHealth Hotlist-GoodFirstBug

Comment 4 by weifangsun@chromium.org, Feb 28 2018

Owner: ----
Status: Unconfirmed (was: Assigned)

Comment 5 by sashab@chromium.org, Feb 28 2018

Labels: -CrOS-FilesApp-CodeHealth CrOSFilesCategory-CodeHealth

Comment 7 by woxxom@gmail.com, Mar 17 2018 

Broken three years ago in Chrome 45.0.2423.0:
r332792 = 1143f7a9f3ada056c5ce7059a553dc6ec18f654f = https://crrev.com/1158923004 by mtomasz@chromium.org

Comment 8 by sashab@chromium.org, Apr 12 2018

Status: Available (was: Untriaged)

Comment 9 by slangley@chromium.org, Oct 18

Owner: adanilo@chromium.org
Status: Assigned (was: Available)

Comment 10 by adanilo@chromium.org, Oct 23

Status: Started (was: Assigned)

Comment 11 by adanilo@chromium.org, Oct 23 

It would be helpful to know which tool generated that output in the bug report.

Comment 12 by yosin@chromium.org, Oct 23 

>#c11, the report in #c1 was generated homemade JavaScript parser[1].
I think it is not easy to use at this time.


[1] https://github.com/eval1749/aoba/tree/master/aoba
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 24 

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

commit 460d22a6f5604d1296073a4c41963f7b889d229b
Author: Alex Danilo <adanilo@chromium.org>
Date: Wed Oct 24 04:58:52 2018

FilesApp: escaped strings in RegExp constructor

File manager added support for using MIME types for icon display
in https://codereview.chromium.org/1158923004 and used a mixture
of string literals and Regexp constructors. The RegExp constructors
need escaping of the strings so they lex correctly. This change
corrects those strings.

BUG= chromium:675506 
TEST=Manually checked, automated lexer in bug report was personal
     project by the reporter.

Signed-off-by: Alex Danilo <adanilo@chromium.org>
Change-Id: I2241b87231d75d0d1b2d2c7b8bdfe13a6d3a0770
Reviewed-on: https://chromium-review.googlesource.com/c/1296934
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602255}
[modify] https://crrev.com/460d22a6f5604d1296073a4c41963f7b889d229b/ui/file_manager/file_manager/common/js/file_type.js
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 24 

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

commit 460d22a6f5604d1296073a4c41963f7b889d229b
Author: Alex Danilo <adanilo@chromium.org>
Date: Wed Oct 24 04:58:52 2018

FilesApp: escaped strings in RegExp constructor

File manager added support for using MIME types for icon display
in https://codereview.chromium.org/1158923004 and used a mixture
of string literals and Regexp constructors. The RegExp constructors
need escaping of the strings so they lex correctly. This change
corrects those strings.

BUG= chromium:675506 
TEST=Manually checked, automated lexer in bug report was personal
     project by the reporter.

Signed-off-by: Alex Danilo <adanilo@chromium.org>
Change-Id: I2241b87231d75d0d1b2d2c7b8bdfe13a6d3a0770
Reviewed-on: https://chromium-review.googlesource.com/c/1296934
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602255}
[modify] https://crrev.com/460d22a6f5604d1296073a4c41963f7b889d229b/ui/file_manager/file_manager/common/js/file_type.js
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 24 

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

commit 63dae32cbb1a4dd78318d07e49d0ac38df691509
Author: Alex Danilo <adanilo@chromium.org>
Date: Wed Oct 24 23:44:28 2018

FilesApp: Correct MIME types for openxmlformats

Commit https://codereview.chromium.org/1158923004 updated
incorrect escapes in some RegExp constructors, but some
of the escapes were superfluous. When checking the formats
it became obvious that the original MIME types used were
incorrect for the family of openxmlformats documents.
This change corrects the MIME types and removes the
unneeded escapes.

BUG= chromium:675506 
TEST=Ran all the FilesApp tests

Signed-off-by: Alex Danilo <adanilo@chromium.org>
Change-Id: Ic79e4655ebb61ea2e7c8fbb83509f899d7d1cb60
Reviewed-on: https://chromium-review.googlesource.com/c/1297775
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602525}
[modify] https://crrev.com/63dae32cbb1a4dd78318d07e49d0ac38df691509/ui/file_manager/file_manager/common/js/file_type.js

Comment 16 by noel@chromium.org, Nov 5 

All work done here?

Comment 17 by adanilo@chromium.org, Nov 5

Status: Fixed (was: Started)
Yes for now, follow on work would be to build some proper unit tests to catch potential regressions...
►

Sign in to add a comment