1281712 - "list index out of range" error while looking at a review request

Status: VERIFIED FIXED

(MozReview Graveyard :: Review Board: Upstream, defect, P1)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Look at this review request: https://reviewboard.mozilla.org/r/60074/diff/1#index_header

Expected: All files should display normally.
Actual: In the file accessible/base/nsAccessibilityService.h, I get:

 There was an error displaying this diff. 
list index out of range
This may be a bug in the software, a temporary outage, or an issue with the format of your diff. 
Please try again, and if you still have trouble, contact support. 

Details show:

Traceback (most recent call last):
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/views.py", line 275, in get
    response = renderer.render_to_response(request)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/renderers.py", line 56, in render_to_response
    return HttpResponse(self.render_to_string(request))
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/renderers.py", line 74, in render_to_string
    large_data=True)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/cache/backend.py", line 295, in cache_memoize
    compress_large_data))
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/cache/backend.py", line 249, in cache_memoize_iter
    items = items_or_callable()
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/cache/backend.py", line 292, in <lambda>
    lambda: [lookup_callable()],
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/renderers.py", line 73, in <lambda>
    lambda: self.render_to_string_uncached(request),
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/renderers.py", line 87, in render_to_string_uncached
    request=request)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/diffutils.py", line 429, in populate_diff_chunks
    chunks = list(generator.get_chunks())
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/chunk_generator.py", line 756, in get_chunks
    for chunk in super(DiffChunkGenerator, self).get_chunks(cache_key):
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/chunk_generator.py", line 107, in get_chunks
    large_data=True)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/cache/backend.py", line 295, in cache_memoize
    compress_large_data))
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/cache/backend.py", line 249, in cache_memoize_iter
    items = items_or_callable()
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/cache/backend.py", line 292, in <lambda>
    lambda: [lookup_callable()],
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/chunk_generator.py", line 106, in <lambda>
    lambda: list(self.get_chunks_uncached()),
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/chunk_generator.py", line 802, in get_chunks_uncached
    for chunk in self.generate_chunks(old, new):
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/chunk_generator.py", line 236, in generate_chunks
    yield self._new_chunk(lines, 0, num_lines, False, tag, meta)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/chunk_generator.py", line 570, in _new_chunk
    compute_chunk_last_header(lines, num_lines, meta, self._last_header)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/diffviewer/chunk_generator.py", line 843, in compute_chunk_last_header
    line = lines[0]
IndexError: list index out of range

Comment 1

[:glob ✱]

i don't see anything obviously broken when looking at the diff on that file on https://reviewboard-hg.mozilla.org/gecko/raw-rev/ca39c48e709a

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm also seeing this on https://reviewboard.mozilla.org/r/54600/diff/2#index_header

Comment 3

[Mark Côté [:mcote]]

Zalun, want to take a look at this?  It's something that should be fixed upstream, but we can do it in our fork first.  This is higher priority than the review-flag stuff, since this is broken functionality.

Comment 4

[Andrew McCreight [:mccr8]]

I'm also seeing this on https://reviewboard.mozilla.org/r/48119/diff/1#5

Comment 5

[Piotr Zalewa [:zalun]]

Tried to replicate it locally. Plain 2 commits to produce the same diff did not work (diff is displayed properly)...

Comment 6

[Piotr Zalewa [:zalun]]

I will try to replicate this one - https://reviewboard.mozilla.org/r/54600/diff/2#index_header - as it's smaller. Might be easier to reproduce

Comment 7

[Piotr Zalewa [:zalun]]

I managed to have this diff locally - https://reviewboard.mozilla.org/r/54600/diff/3/ 
It is working perfectly fine.

Comment 8

[Piotr Zalewa [:zalun]]

Update. To replicate I tried following and failed:

1. Create a diff including only the file which is broken

2. Create a diff by downloading all files included in review request

3. Cloned the project (used the review request from comment #2), updated from the target revision and pushed to local reviewboard. 

  hg clone https://hg.mozilla.org/mozilla-central
  cd mozilla-central
  hg pull -r 950c511420f066b834e28282abf53187299e714c https://reviewboard-hg.mozilla.org/gecko
  hg update
  cp ../local-mozreview-test-repo/.hg/hgrc ./.hg/
  hg commit --amend "bug 11"
  hg push
  
We (+mcote +glob) also have found few ideas which might work in the future:

1. Try to push the code to dev reviewboard server instead of the local one

2. Download broken diff from live database and plant it in local one.
It might be difficult to find the right row in database. The ids in UI seem to be created in order, so especially the #c2 review request should be helpful. It might be enough to check id of the diff before and after the broken one and look for diffs between them.

Last idea could lead us to create a testable workaround. If the exception is raised we could create a diff live and place a fixed one in database. We should definitely inform about the issue by sending a message to project administrator.

Comment 10

[Mark Côté [:mcote]]

Got it again in https://reviewboard.mozilla.org/r/98150/diff/2#index_header.  We really need to figure this out.

Comment 11

[Israel Madueme [:imadueme]]

Another one: https://reviewboard.mozilla.org/r/106440/diff/1#index_header
From gerald

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: mozreview-breaker from bug 1332568

Even after a rebase & push, I'm getting the same result:
https://reviewboard.mozilla.org/r/106440/diff/2#index_header

I'm attaching the patch as a file here, if that helps.

Comment 14

[Chris Peterson [:cpeterson]]

I hit this error in the following review requsts. There seems to be something about the file widget/windows/nsWinGesture.h that makes mozreview unhappy.

https://reviewboard.mozilla.org/r/110176/diff/1#index_header
https://reviewboard.mozilla.org/r/109910/diff/2#index_header

Comment 15

[Piotr Zalewa [:zalun]]

Attachment: Workaround to display failed filediffs

Comment 16

[Piotr Zalewa [:zalun]]

All of the failing filediffs have last chunk empty.

Calling compute_chunk_last_header only for non-empty lines prevents 500 
`compute_chunk_last_header(lines, num_lines, meta, self._last_header)`.
Attached patch is a workaround which does exactly that.

We should also log all such cases for further debugging of the root cause.

Comment 17

[Mark Côté [:mcote]]

That's an interesting discovery.  Did you dig into compute_chunk_last_header() to see where this condition (0 lines) could possibly be causing an exception?  Also were you able to reproduce this locally, given that you now know what triggers it?

Comment 18

[Piotr Zalewa [:zalun]]

As in the initial message, it's trying to get lines[0].
I'm more observing it than reproducing, as I've failed to create such a filediff with a push to test repository. Although for now I've  been more focused on creating this fix.
Issue is visible locally when using the sanitized db. On the same urls as provided in this bug.

Comment 19

[Mark Côté [:mcote]]

Right, my question was whether there was some code path that was erroneously being followed that led compute_chunk_last_header() to try to access lines[0].  Looking at the code I see that it's one of the first lines, so I guess not.

Where are you planning to put the new log statements?

Comment 20

[Piotr Zalewa [:zalun]]

To avoid further modifications of the RawDiffChunkGenerator object (incrementing _chunk_index and passing further the headers) it would be good to log the error and then return with a fake chunk containing minimal information. Following code is working for my local environment on all the replicable issues mentioned in comments above:

if num_lines == 0:
    logger.error('No lines in last chunk issue. Bug 1281712 %s'
                  % self.filediff)
    return {
        'change': tag,
        'numlines': 0,
        'lines': [],
    }
else:
    compute_chunk_last_header(lines, num_lines, meta,
                              self._last_header)

It would be good if :smacleod or :glob would look at it if this is not breaking something I don't see.

The contents of the problematic last chunk looks like this:
{'index': 26, 
 'lines': [], 
 'numlines': 0, 
 'meta': {
     'right_headers': [], 
     'whitespace_chunk': False, 
     'left_headers': [], 
     'whitespace_lines': []}, 
 'collapsable': False, 
 'change': 'equal'}

Comment 21

[Piotr Zalewa [:zalun]]

Attachment: A typo in options for pygments lexer

Comment 22

[Piotr Zalewa [:zalun]]

We've got a typo in options for pygments. 
Pushing the review request.

Comment 23

[Piotr Zalewa [:zalun]]

Attachment: mozreview: Fix pygments options to not strip \n from files (bug 1281712)

`pygments` lexer accepts a Boolean `stripnl` option to [not] remove \n from files. We've got it spelled as "stripln" in `overridable_chunk_generator.py` file.

This causes issues with displaying the filediffs. In `chunk_generator.py#843` we assume `lines` is a non-empty list. If there was an empty line at the end of the file, `pygments` removed it entirely. As a result, `markup_a` and `markup_b` - lists returned from lexer are shorter by that empty line. `lines` are created by slicing markup files with `i1`, `i2`, `j1`, `j2` - opcodes generated with `diff_opcode_generatos` operating on files before lexer was run. `markup_a[i1:i2]` and `markup_b[j1:j2]` where empty lists for the last chunk.

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

Comment 24

[Piotr Zalewa [:zalun]]

Comment on attachment 8839159 [details]
mozreview: Fix pygments options to not strip \n from files (bug 1281712)

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

Comment 25

[Steven MacLeod [:smacleod]]

Comment on attachment 8839159 [details]
mozreview: Fix pygments options to not strip \n from files (bug 1281712)

https://reviewboard.mozilla.org/r/113880/#review115954

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6a5b4c985ffc
mozreview: Fix pygments options to not strip \n from files r=smacleod

Comment 27

[Mark Côté [:mcote]]

This was pushed out to production, and all the above review-request links now work fine, with no errors.