820: It is possible for a user to create multiple draft reviews, leading to an unviewable review

timw.a******@gmai***** (Google Code) (Is this you? Claim this profile.)
chipx86
chipx86
Jan. 24, 2009
What's the URL of the page containing the problem?
"View diff" page on another user's published review

What steps will reproduce the problem?
1. Comment on multiple lines in the diff
2. Edit some of your comments (by clicking on the comment markers, changing
the text, and hitting Save)
3. Hit "Edit Review" on the green draft-review banner
4. Fill in some review text and change one or more of the comments
5. Hit "Publish Review"

I have not minimized the steps, yet, but I've only just encountered this
recently (and once). It may be necessary to further click on some
just-entered comments and click cancel.

What is the expected output? What do you see instead?
Expected:
The (single) review I have been composing is published, and an email is sent.

Actual:
An email is sent for the review I expected, but the publish action
redirects me to an exception page ('get()' returning multiple rows instead
of 1). The account who just published the review can no longer access that
review (the same exception page shows).

What operating system are you using? What browser?
Ubuntu 8.10 x86_64's Firefox 3.0.5

Please provide any additional information below.
If it matters, there were other reviews (with code references) on an older
diff. I think I did not click on any markers for these older reviews, since
I was reviewing a newer diff with no existing reviews.

Unfortunately, I didn't capture the stack trace.
It is likely easy to reproduce the stack trace: just use the Admin tool to
toggle the 'Public' field to 'false' for at least two reviews from the same
user in the same review request, then try to view the review request as
that user.

The real issue seems to be caused by several factors:
1) get() is used when retrieving a user's draft review(s) for a given
review request,
2) the JSON API and/or the underlying model does not prevent the creation
of multiple (user, review request, non-Public review) sets, and
3) some of the JS in the diff viewer page is (accidentally or
intentionally) trying to create a new review request instead of modifying
the current draft in at least one situation.

If multiple drafts are reasonable, (1) is the only problem, and some GUI
work may be in order.
It seems to me that the real problem is (3).
The existence of (2) allows (3) to violate the model's invariants so that
it becomes impossible for the user to fix it.

Workaround:
If you encounter this (an exception when trying to view a review), either
(1) view it anonymously, or (2) log in as Administrator and find the
offending reviews (either delete them or mark them as Public).
#1 timw.a******@gmai***** (Google Code) (Is this you? Claim this profile.)
This is probably a duplicate of issue 709 and/or issue 705.

I now have a tcpdump capture of the problem, but this time it did not result in an
exception (instead, it resulted in two reviews posted in close succession, where one
had a small subset of comments from the other). I cannot make that file available, as
it contains proprietary data, but I can answer questions about it.

I did notice that there were *two* (2) POSTs to the same URL for seemingly all of the
comments.

Digging further (into the capture, not the code):
1) enter a comment, hit save
2) a JSON post saves the comment
3) time passes... (more reviewing, commenting, etc.)
4) publish the review
5) every comment you have posted before is re-posted (same URL and post body as
before) as fast the browser, server, and network allows (mine covers 30 comments in
the span of 10 seconds)
6) less than 1 second after the final comment is posted again, the draft is published.

Factors that probably play a role for me:
1) I am on a LAN (high speed, low latency)
2) I am doing this testing at a low-usage time (the review server is idle)
3) I am using apache2 with the prefork MPM with: StartServers=5, MinSpareServers=5,
MaxSpareServers=10, MaxClients=150, MaxRequestsPerChild=0
4) I am working on a large review (about 40 files; at least that many comments;
comments ranging from one-liners to multi-paragraph ones)
5) I am composing that review over the course of an hour or two (so the server has
time to return to an idle state most of the time)

If the real fix is challenging, a short-term hack may be to have a new JSON API to
wait-until-draft-settled: given a review request (user+review_request pair to the
server), it will not return success until the draft has been idle for some specified
time window (client-specified, possibly hard-limited to 30 seconds to avoid abuse).

The algorithm might look something like (simplified for brevity):
1) gather state of the draft
2) wait until requested time elapses
3) gather state of draft
4) if draft state at 3 is different from draft state at 1, go to 1

This way, the attempt to publish will not go through until all in-flight comment
posts have completed.

An even simpler hack may be to simply make the client code pause for N seconds after
reposting the last comment before posting the publish command.

It seems a little surprising (to me) that the post-comment API would return before it
has been committed, but there may be complicating factors involved.
chipx86
#2 chipx86
I've recently noticed this as well. We need to make this a priority for alpha 2.

Thanks for all the research on it. I'm going to go through later and see if I can get
a solid repro case and get this fixed.
  • +Confirmed
  • -Priority-Medium
    +Priority-Critical
    +Milestone-Release1.0
  • +chipx86
chipx86
#3 chipx86
So we're trying to be smart by setting up an asynchronous chain for the saves, so
that we save one comment at a time, wait for it to save, and then move on to the
next, and finally publish at the end.

Unfortunately, we're listening for when the inlineEditor's "complete" signal, which
doesn't mean we've saved to the server yet. So we end up with a race condition where
we're pushing all comment saves *and* publishing the review at the same time. The
solution is to have a custom signal emitted when we know the actual data has been
saved to the server.
chipx86
#4 chipx86
This should be fixed now in r1703. Let me know if you still hit it.

Thanks!
  • -Confirmed
    +Fixed
  • +Component-Reviews