1262648 - browser_misused_characters_in_strings.js doesn't work correctly with string definitions spanning multiple lines

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

From bug 1259859 comment 34:

It appears this test doesn't work for strings that contain newline characters in their definitions. I noticed this after a try push for bug 1240594 (which removes aboutCertError.dtd).

Specifically, that bug moves the certerror.introPara string to netError.dtd and splits it into 3 lines. I changed the whitelist to reflect that this string is now in netError.dtd, but I ended up with a failure because it never got removed from the whitelist.

Keeping the entire string in one line eliminates this failure. This seems like a bug in the test to me, because afaik string definitions in DTDs can span multiple lines.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: MozReview Request: Bug 1262648 - Make ENTITY matching regexp allow multiline string values in browser_misused_characters_in_strings.js. r=jaws

Review commit: https://reviewboard.mozilla.org/r/44721/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44721/

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

So, my apologies for the crazy regex I ended up with.

I started out just changing a "." to "[\s\S]" so that newlines are matched, but that led to greedy vs lazy behavior to correctly match within the same entity, and then some other issue, and another one.... and I ended up just writing a proper comprehensive regex to match full ENTITY tags.

You can see and play with my test cases here: http://www.regexpal.com/?fam=94585

If you think this is overkill (which I would agree with) I'll see if I can start over and come up with something simple that works with our codebase (i.e. making assumptions about code style).

Comment 3

[Axel Hecht [:Pike]]

Don't make assumptions, l10n is fuzz testing those.

https://dxr.mozilla.org/mozilla-central/source/python/compare-locales/compare_locales/parser.py#215 has a decent parser, we have a few years of experience with that one.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

https://reviewboard.mozilla.org/r/44721/#review41505

Found a much more straightforward, obvious regex.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8738859 [details]
MozReview Request: Bug 1262648 - Make ENTITY matching regexp allow multiline string values in browser_misused_characters_in_strings.js. r=jaws

Found a better regex

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8738859 [details]
MozReview Request: Bug 1262648 - Make ENTITY matching regexp allow multiline string values in browser_misused_characters_in_strings.js. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44721/diff/1-2/

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

New regex is adapted from the file Axel linked to. It doesn't work when there are escaped quotes within the string value (e.g. <!ENTITY some.key "<span class=\"some value\">">), but since it seems we've been using that parser for a while now I think the precedent and simplification of the regex justify the lack of support for that edge case. Also I could not find any such example in our current code base [1].

[1] https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A<!ENTITY\s%2B[\w\.]%2B\s%2B("[^"]*\\"[^"]*"|'[^']*\\'[^']*')\s*>

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8738859 [details]
MozReview Request: Bug 1262648 - Make ENTITY matching regexp allow multiline string values in browser_misused_characters_in_strings.js. r=jaws

https://reviewboard.mozilla.org/r/44721/#review41597

Please add a dummy DTD file that is opened by the test and confirms that we will support multiline strings even if we aren't shipping any.

The rest of the patch looks fine.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8738859 [details]
MozReview Request: Bug 1262648 - Make ENTITY matching regexp allow multiline string values in browser_misused_characters_in_strings.js. r=jaws

https://reviewboard.mozilla.org/r/44721/#review42413

r=me if you add a new DTD that is opened by the test to confirm that multiline strings are interpreted correctly.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8738859 [details]
MozReview Request: Bug 1262648 - Make ENTITY matching regexp allow multiline string values in browser_misused_characters_in_strings.js. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44721/diff/2-3/

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

r=me on the updated patch

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

https://hg.mozilla.org/integration/fx-team/rev/5ad0f8d2d7dffcf57ff4d935f5dfb4781683091e
Bug 1262648 - Make ENTITY matching regexp allow multiline string values in browser_misused_characters_in_strings.js. r=jaws

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ad0f8d2d7df