Commit d697ac9f authored by Mustafa Emre Acer's avatar Mustafa Emre Acer Committed by Commit Bot

Fix crash in translation screenshot presubmit

The presubmit code for translation screenshots parses grd and grdp files
modified in the CL. It does this by referring to the old and new
contents of the files passed to it by the presubmit.

grd files can refer to grdp files via <part> tags. This causes the
messages in the grdp file to be included in the grd file. If a modified
grd file's old contents refers to a now renamed or deleted grdp file,
grd reader fails to open the grdp and crashes.

This CL fixes this by ignoring <part> tags while loading grd files.
This should be safe: The presubmit processes grdp files separately, so
all grdp files should still be processed, just not when loading grd
files. Additionally, grdp files cannot contain part files, so
ignoring the tag while loading grdp files should also be safe.

Bug: 1047487
Change-Id: I3d79aa028c4ae360d74a437a019628d0bdab494a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037027Reviewed-by: default avataranthonyvd <anthonyvd@chromium.org>
Reviewed-by: default avatarJochen Eisinger <jochen@chromium.org>
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739040}
parent a5176675
......@@ -4752,6 +4752,13 @@ def _CheckTranslationScreenshots(input_api, output_api):
file_path = f.LocalPath()
old_id_to_msg_map = {}
new_id_to_msg_map = {}
# Note that this code doesn't check if the file has been deleted. This is
# OK because it only uses the old and new file contents and doesn't load
# the file via its path.
# It's also possible that a file's content refers to a renamed or deleted
# file via a <part> tag, such as <part file="now-deleted-file.grdp">. This
# is OK as well, because grd_helper ignores <part> tags when loading .grd or
# .grdp files.
if file_path.endswith('.grdp'):
if f.OldContents():
old_id_to_msg_map = grd_helper.GetGrdpMessagesFromString(
......
......@@ -23,7 +23,14 @@ TAGS_TO_IGNORE = (
# strings.
'include',
# <structure> tags point to image files.
'structure')
'structure',
# <part> tags point to .grdp files. Don't load included part files when
# loading a .grd or .grdp file. A grd file's contents can refer to deleted
# grdp files (e.g. if a grdp file was renamed). Trying to load it would
# fail. It's also unnecessary to load <part> files because grdp files are
# handled separately in GetGrdpMessagesFromString. Grdp files are also
# expected to not contain any <part> tags.
'part')
def GetGrdMessages(grd_path_or_string, dir_path):
......
......@@ -28,11 +28,12 @@ class GrdHelperTest(unittest.TestCase):
grd_dir = os.path.join(repo_root, 'tools', 'translation', 'testdata')
messages = grd_helper.GetGrdMessages(
os.path.join(grd_dir, 'test.grd'), grd_dir)
# Result should contain all messages from test.grd and part.grdp.
# Result should contain only the messages in test.grd. Even though the file
# includes part.grdp via a <part> tag, part.grdp's messages should not be
# listed here.
self.assertTrue('IDS_TEST_STRING1' in messages)
self.assertTrue('IDS_TEST_STRING2' in messages)
self.assertTrue('IDS_TEST_STRING_NON_TRANSLATEABLE' in messages)
self.assertTrue('IDS_PART_STRING_NON_TRANSLATEABLE' in messages)
def testReadGrdpMessages(self):
messages = grd_helper.GetGrdpMessagesFromString(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment