831168 - mach should handle a corrupted warnings database

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

During a build, the filesystem ran out of free space. Mach died when writing the compilation database. No big deal there.

The problem was the next time I ran |mach build|:

Error running mach:

    ['build']

The error occured in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ValueError: Unterminated string starting at: line 6279 column 22 (char 266212)

  File "/home/gps/src/build-system/python/mozbuild/mozbuild/mach_commands.py", line 35, in build
    warnings_database.load_from_file(warnings_path)
  File "/home/gps/src/build-system/python/mozbuild/mozbuild/compilation/warnings.py", line 264, in load_from_file
    self.deserialize(fh)
  File "/home/gps/src/build-system/python/mozbuild/mozbuild/compilation/warnings.py", line 243, in deserialize
    obj = json.load(fh)
  File "/usr/lib/python2.7/json/__init__.py", line 280, in load
    **kw)
  File "/usr/lib/python2.7/json/__init__.py", line 328, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 365, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 381, in raw_decode
    obj, end = self.scan_once(s, idx)

IMO we should automatically replace corrupted on unreadable databases. Perhaps we spit out a warning in the process.

Comment 1

[Zuhao(Joe) Chen]

Attachment: A quick fix try to fix this bug.

A quick fix try to fix this bug by adding a verification method before reading database file into json.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 705242 [details] [diff] [review]
A quick fix try to fix this bug.

Review of attachment 705242 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the effort. However, I'd prefer that we avoid double loading of the file. I would either wrap load_from_file() in a try..except everywhere or I would modify load_from_file() to trap exceptions and return a boolean.

If you modify warnings.py, a unit test would also be nice. See the test_warnings.py file. You can execute all the tests by running from your objdir: |make -C python check|

Comment 3

[Zuhao(Joe) Chen]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 705242 [details] [diff] [review]
> A quick fix try to fix this bug.
> 
> Review of attachment 705242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the effort. However, I'd prefer that we avoid double loading
> of the file. I would either wrap load_from_file() in a try..except
> everywhere or I would modify load_from_file() to trap exceptions and return
> a boolean.
> 
> If you modify warnings.py, a unit test would also be nice. See the
> test_warnings.py file. You can execute all the tests by running from your
> objdir: |make -C python check|

Thank you for the feedback! You're right, it's kinda weird to load it twice. Let me try work around it.

Comment 4

[Zuhao(Joe) Chen]

Attachment: Rework after Gregory's feedback

Do I need a unit test for mach_command? It seems there is not test related to this module.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 705469 [details] [diff] [review]
Rework after Gregory's feedback

Review of attachment 705469 [details] [diff] [review]:
-----------------------------------------------------------------

No, we don't need a unit test for the mach_commands.py file at this time.

I suppose this patch could be more thorough: we may see other types of exceptions and os.remove may fail. But, I'm not too worried about these. This bug was already an edge case. So, the implementation should be good enough. If we see additional failures, we'll deal with them.

Comment 6

[Zuhao(Joe) Chen]

Attachment: bug 831168

Comment 7

[Gregory Szorc [:gps]]

I'm going to just set the whiteboard since I'm pretty pretty. Someone will magically come along and upload it within a few hours.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d54dfb62030

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to Gregory Szorc [:gps] from comment #7)
> I'm going to just set the whiteboard since I'm pretty pretty.

Well you are a handsome man, but no need to inflate your own ego.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d54dfb62030

Comment 11

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> (In reply to Gregory Szorc [:gps] from comment #7)
> > I'm going to just set the whiteboard since I'm pretty pretty.
> 
> Well you are a handsome man, but no need to inflate your own ego.

Sometimes I accidentally type words multiple multiple.