Closed Bug 1149176 Opened 6 years ago Closed 6 years ago

Errors from expired sessions don't bubble up to prevent the action

Categories

(MozReview Graveyard :: General, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mconley, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

jimm gave me a review on a child commit here:

https://reviewboard.mozilla.org/r/6163/

I expected that review to have its comments mirrored to the bug (bug 1148012).

But no mirroring occurred.
Aha, I see this in the log.  It *is* due to expired tokens.  Gr.  We should fix this to actually stop the publishing from happening; even with API keys, we want to detect when one has been invalidated.

14:53:45 	ERROR 	

 - Error when calling <function on_review_publishing at 0x7f008f16db18> from SignalHook: Publish error: Bugzilla error: The cookies or token provide were not valid or have expired. You may login again to get new cookies or a new token.
Traceback (most recent call last):
  File "/data/www/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/Djblets-0.8.15-py2.6.egg/djblets/extensions/hooks.py", line 196, in _wrap_callback
    self.callback(extension=self.extension, **kwargs)
  File "build/bdist.linux-x86_64/egg/rbbz/extension.py", line 180, in _transform_errors
    raise PublishError('Bugzilla error: %s' % e.msg)
PublishError: Publish error: Bugzilla error: The cookies or token provide were not valid or have expired. You may login again to get new cookies or a new token.
Priority: -- → P1
Is the workaround for this to log out of Review Board and log back in?
Yes.
Duplicate of this bug: 1146412
Better summary of the problem.  We also have a very easy fix! \o/
Assignee: nobody → mcote
Status: NEW → ASSIGNED
Summary: Some comments not getting mirrored to Bugzilla → Errors from expired sessions don't bubble up to prevent the action
Attached file MozReview Request: bz://1149176/mcote (obsolete) —
/r/7009 - Bug 1149176 - Don't sandbox errors in hooks that talk to Bugzilla. r=smacleod

Pull down this commit:

hg pull -r 4bb3af0573898f0b2e2810466c3f418bf80d89dd https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8591848 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/7009/#review5817

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
>          SignalHook(self, review_request_closed,
> -                   on_review_request_closed_discarded)
> +                   on_review_request_closed_discarded,
> +                   sandbox_errors=False)

I don't think this is an abortable signal, we want the sandboxing here.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +        # Any signal hooks that talk to Bugzilla should have
> +        # sandbox_errors=False, since we don't want to complete the action if

any "abortable" signal maybe?
Attachment #8591848 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/7009/#review5821

> I don't think this is an abortable signal, we want the sandboxing here.

Ah shoot, you're right.  I think we *should* be using an abortable signal here, but that's a separate bug.
Comment on attachment 8591848 [details]
MozReview Request: bz://1149176/mcote

/r/7009 - Bug 1149176 - Don't sandbox errors in hooks that talk to Bugzilla. r=smacleod

Pull down this commit:

hg pull -r a58f73fba8c2a11d10446f8cbc71603d391148df https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8591848 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/7009/#review5919

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +

Get rid of this blank line.
Comment on attachment 8591848 [details]
MozReview Request: bz://1149176/mcote

https://reviewboard.mozilla.org/r/7007/#review5921

Fix it, then Ship-it!
Attachment #8591848 - Flags: review?(smacleod) → review+
http://hg.mozilla.org/hgcustom/version-control-tools/rev/a6dc54687ced

This should be deployed to production early next week, hopefully on Monday.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Is there a bug on file to find some way of refreshing the token to avoid this problem entirely?
Flags: needinfo?(mcote)
Not really, since you have to log in with your password, and we don't store that.

However, we will be switching to API keys once bug 1144468 is fixed to allow easy authorization of MozReview, and those won't expire (unless the user explicitly invalidates it).  So that should solve the problem properly.
Flags: needinfo?(mcote)
Attachment #8591848 - Attachment is obsolete: true
Attachment #8619913 - Flags: review+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.