1315395 - 500 ISE errors when dealing with patches that use tabs to change the indentation of existing lines

Reporter

Description

•

8 years ago

Reported by :jaws in #moc.

<jaws> hey #moc, i'm getting HTTP 500 errors on mozreview when trying to publish review feedback
<jaws> i just get a message box with "HTTP 500 INTERNAL SERVER ERROR"
<jaws> yeah i get a 500 when i click the Publish button on this review, https://reviewboard.mozilla.org/r/90168/diff/1#index_header

On reviewboard2.webapp.scl3.mozilla.com:/var/log/reviewboard/reviewboard.log I found the following, followed by some python errors:

4523:2016-11-04 22:55:54,731 - INFO -  - Publishing review for user: jaws review id: 90536 review request id: 90168
4539:2016-11-04 22:55:55,253 - ERROR -  - Exception thrown for user jaws at https://reviewboard.mozilla.org/api/review-requests/90168/reviews/90536/
4599:2016-11-04 22:56:26,912 - INFO -  - Publishing review for user: jaws review id: 90536 review request id: 90168
4615:2016-11-04 22:56:27,167 - ERROR -  - Exception thrown for user jaws at https://reviewboard.mozilla.org/api/review-requests/90168/reviews/90536/
4686:2016-11-04 22:59:21,931 - INFO -  - Publishing review for user: jaws review id: 90536 review request id: 90168
4702:2016-11-04 22:59:22,054 - ERROR -  - Exception thrown for user jaws at https://reviewboard.mozilla.org/api/review-requests/90168/reviews/90536/
4757:2016-11-04 23:08:17,597 - INFO -  - Publishing review for user: jaws review id: 90536 review request id: 90168
4773:2016-11-04 23:08:17,785 - ERROR -  - Exception thrown for user jaws at https://reviewboard.mozilla.org/api/review-requests/90168/reviews/90536/

2016-11-04 22:56:27,162 - ERROR -  - Error when calling <function on_review_publishing at 0x7f44dedf3aa0> from SignalHook: pop from empty list

Keegan Ferrando [:fauweh]

Reporter

Updated

•

8 years ago

Summary: 500 ISE errors when posting review → 500 ISE errors when posting review to reviewboard.m.o

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

Comment 1

•

8 years ago

I'm still getting this error today on the same review. It won't let me publish it. Any work-arounds?

Flags: needinfo?(kferrando)

Kendall Libby [:fubar] (he/him)

Updated

•

8 years ago

Group: infra → mozilla-employee-confidential

Product: Developer Services → MozReview

Kendall Libby [:fubar] (he/him)

Comment 2

•

8 years ago

Two tracebacks for the prices of one:

Traceback (most recent call last):
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/extensions/hooks.py", line 196, in _wrap_callback
    self.callback(extension=self.extension, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/bugzilla/errors.py", line 28, in _transform_errors
    return func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/signal_handlers.py", line 652, in on_review_publishing
    {"user": user})
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/diffs.py", line 337, in build_plaintext_review
    review.is_reply()))
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/diffs.py", line 310, in render_comment_plain
    lines.extend(render_equal_chunk(chunk, parser))
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/diffs.py", line 191, in render_equal_chunk
    replace_chars.pop()
IndexError: pop from empty list

... and ...

Traceback (most recent call last):
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/handlers/base.py", line 112, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/lib64/python2.6/site-packages/newrelic-2.44.0.36/newrelic/hooks/framework_django.py", line 497, in wrapper
    return wrapped(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/views/decorators/cache.py", line 52, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/views/decorators/vary.py", line 19, in inner_func
    response = func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/resources/base.py", line 196, in __call__
    request, method, view, api_format=api_format, *args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/resources/mixins/api_tokens.py", line 65, in call_method_view
    return view(request, *args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/resources/base.py", line 464, in put
    return self.update(request, *args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/decorators.py", line 122, in _call
    return view_func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/ReviewBoard-2.5.4.moz-py2.6.egg/reviewboard/webapi/decorators.py", line 139, in _check
    return view_func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/decorators.py", line 122, in _call
    return view_func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/decorators.py", line 143, in _checklogin
    return view_func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/decorators.py", line 122, in _call
    return view_func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/decorators.py", line 122, in _call
    return view_func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/webapi/decorators.py", line 307, in _validate
    return view_func(*args, **new_kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/ReviewBoard-2.5.4.moz-py2.6.egg/reviewboard/webapi/resources/base_review.py", line 256, in update
    return self._update_review(request, review, *args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/ReviewBoard-2.5.4.moz-py2.6.egg/reviewboard/webapi/resources/base_review.py", line 303, in _update_review
    review.publish(user=request.user)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/ReviewBoard-2.5.4.moz-py2.6.egg/reviewboard/reviews/models/review.py", line 222, in publish
    review=self)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/dispatch/dispatcher.py", line 185, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/djblets/extensions/hooks.py", line 196, in _wrap_callback
    self.callback(extension=self.extension, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/bugzilla/errors.py", line 28, in _transform_errors
    return func(*args, **kwargs)
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/signal_handlers.py", line 652, in on_review_publishing
    {"user": user})
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/diffs.py", line 337, in build_plaintext_review
    review.is_reply()))
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/diffs.py", line 310, in render_comment_plain
    lines.extend(render_equal_chunk(chunk, parser))
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/diffs.py", line 191, in render_equal_chunk
    replace_chars.pop()
IndexError: pop from empty list

Flags: needinfo?(kferrando)

:glob ✱

Assignee

Comment 3

•

8 years ago

looks like something about that patch is breaking the handling of the indentation.
that patch is unusual because it contains tabs and some lines are CRLF delimited.

:glob ✱

Assignee

Comment 4

•

8 years ago

i can reproduce this issue in my dev environment; working on it.

Assignee: nobody → glob

Group: mozilla-employee-confidential

Summary: 500 ISE errors when posting review to reviewboard.m.o → 500 ISE errors when dealing with patches that use tabs to change the indentation of existing lines

Comment hidden (mozreview-request)

Lines indented with tabs instead of spaces shouldn't cause an Internal Server
Error.  Fix the bug, update tab/mdash handling to handle an upstream change,
and tidy up the code slightly.

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

Steven MacLeod [:smacleod]

Comment 6

•

8 years ago

mozreview-review

Comment on attachment 8808483 [details]
mozreview: fix handling of tab-indented lines (bug 1315395);

https://reviewboard.mozilla.org/r/91316/#review91590

Thanks for fixing this up, sooo much nicer now.

::: pylib/mozreview/mozreview/diffs.py:188
(Diff revision 1)
> -            elif chars[index] == "&" and chars[index + 1] == "m":  # "&mdash;".
> -                # One of the spaces we translated before
> -                # was actually part of this tab we've
> +                # Skip &gt; prefix.
> +                if chars[index:index + 4] == "&gt;":
> +                    index += 4

I had thought these characters were actually part of the tabs length, should we not be incrementing the `current_width`?

::: pylib/mozreview/mozreview/diffs.py:197
(Diff revision 1)
> +                # Skip &gt; suffix.
> +                if chars[index:index + 4] == "&gt;":
> +                    index += 4

again, should we not increment the `current_width`?

::: pylib/mozreview/mozreview/diffs.py:216
(Diff revision 1)
> +                if index > 0:
> -                replace_chars.pop()
> +                    replace_chars.pop()

Is there really a case where this triggers? or are you just being defensive?

Attachment #8808483 - Flags: review?(smacleod) → review+

:glob ✱

Assignee

Comment 7

•

8 years ago

mozreview-review-reply

Comment on attachment 8808483 [details]
mozreview: fix handling of tab-indented lines (bug 1315395);

https://reviewboard.mozilla.org/r/91316/#review91590

> Is there really a case where this triggers? or are you just being defensive?

defensive - should the rb formatting change again i'd prefer to output the wrong number of spaces in bugzilla than crash.

Comment hidden (mozreview-request)

Comment on attachment 8808483 [details]
mozreview: fix handling of tab-indented lines (bug 1315395);

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

Pulsebot

Comment 9

•

8 years ago

Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/ba836bfdca9a
mozreview: fix handling of tab-indented lines ; r=smacleod

Status: NEW → RESOLVED

Closed: 8 years ago

Resolution: --- → FIXED

Bugzilla

Quick Search

500 ISE errors when dealing with patches that use tabs to change the indentation of existing lines

Categories

(MozReview Graveyard :: General, defect)

Tracking

(Not tracked)

People

(Reporter: fauweh, Assigned: glob)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Updated

Comment 1

Updated

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Attachment

General

Description

File Name

Content Type