Here's a recent dependency mail I just received: Subject: [Bug 394887] Consider creating widgets for nsMenuPopupFrames lazily Date: Tue, 18 Sep 2007 07:32:45 -0700 Do not reply to this email. You can add comments to this bug at https://bugzilla.mozilla.org/show_bug.cgi?id=394887 Bug 394887 depends on bug 396412, which changed state. Bug 396412 Summary: Build 2007091623 crash [@ nsMenuPopupFrame::MoveToInternal] when using Ctrl-Tab extension https://bugzilla.mozilla.org/show_bug.cgi?id=396412 What |Old Value |New Value ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Status|NEW |ASSIGNED Status|ASSIGNED |RESOLVED Resolution| |FIXED Notice the three different statuses listed as being changed. If you look at https://bugzilla.mozilla.org/show_activity.cgi?id=396412, you'll see that the status did not change that quickly. The only status change that should have been shown was ASSIGNED going to RESOLVED FIXED. I've been meaning to file this for months, but I kept forgetting to do so. :(

I don't see how this could be a major issue. I know there is a bug somewhere about it, but I cannot find it right now.

So, this is really annoying, and I see this all the time. Can we get this fixed for 3.0 branch or at least tip (so I can backport the fix to bmo)?

I haven't seen this very much or at all. Until there's some analysis of why it's happening (or if it's even really a serious bug), I can't mark this a blocker.

(In reply to comment #3) > I haven't seen this very much or at all. Until there's some analysis of why > it's happening (or if it's even really a serious bug), I can't mark this a > blocker. This happens because the dependency bug uses its own 'lastdiffed' value to get the status and resolution changes in the other bug. So if the blocking bug changed its status or resolution 5 times since the last time the blocker was touched, then you get all 5 changes in the bugmail, see BugMail.pm around lines 218-219: $when_restriction = ' AND bug_when > ? AND bug_when <= ?'; push @args, ($start, $end); and around lines 288+: my $dependency_diffs = $dbh->selectall_arrayref( "SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name, ... WHERE dependencies.blocked = ? AND (fielddefs.name = 'bug_status' OR fielddefs.name = 'resolution') $when_restriction Where $when_restriction uses $start and $end which are those of your current bug, as explained above. The reason we, Bugzilla developers, don't see it very much is because we mark regressions as *depending* on the bug which introduced the regression, and so we don't see a lot of activity in the dependent bug (which is already closed). The reason Firefox developers (and some others) see it a lot is because they mark regressions as *blocking* the bug which introduced the regression, and this "regression bug" gets activity, generally (UNCO ->) NEW -> ASSIGNED -> RESOLVED and so when the regression is fixed, the bug which introduced the regression will notify its assignee, QA contact and CC list about bug status and resolution changes in the "regression bug" since the last time the original bug was touched, i.e. since the "regression bug" was marked as blocking the original bug. This explains why users in the bug which introduced the regression sees the whole history of the newer bug. I see two ways to fix the problem: 1) process_bug.cgi passed the $timestamp to BugMail.pm when calling send_results(). This way, BugMail.pm will only look for changes since $timestamp, i.e. |SELECT NOW()|, i.e. will only get the very last bug status and resolution change. If $timestamp is not passed to BugMail.pm, then BugMail::Send() should fallback to bugs.lastdiffed as it currently does. 2) After the SQL call around line 288 shown above, use pop(@$dependency_diffs) twice to extract the *last* change to the bug status and resolution of the dependent bug. Solution 2) above is probably safer (and less invasive) as we are sure we don't miss changes made to the bug between bugs.lastdiffed and $timestamp in case the MTA was unable to send bugmails for some time (e.g. due to heavy load). My opinion is that this is *not* a blocker. This behavior is present for years, even in Bugzilla 2.16 or 2.18.

This is going to be pretty invasive, and this is a minor annoyance, so I don't think that's something for 4.0.

The hardcoded strings will go away in bug 466968. For now, this highly simplifies the code in BugMail.pm, and will make the next step much easier.

Comment on attachment 459507 [details] [diff] [review] patch, v1 >Index: Bugzilla/BugMail.pm >+ # Only used for headers in bugmail for new bugs >+ my @fields = $start ? () : Bugzilla->get_fields({obsolete => 0, mailhead => 1}); Hmm. If it's really only used there, how about we just move it closer to that block, or inside that block (if possible)? Also, I noticed that inside of _get_diffs you started to use $bug->lastdiffed instead of $start--should we just be using $bug->lastdiffed everywhere instead of $start? >+ my ($diffparts, $changedfields, $diffs) = >+ $params->{dep_only} ? ([], [], []) : _get_diffs($bug, $end); >+ my $deptext = >+ "\nBug " . $bug->id . " depends on bug $blocker_id, which changed state.\n\n" . >+ "Bug $blocker_id Summary: " . $params->{blocker}->short_desc . "\n" . >+ correct_urlbase() . "show_bug.cgi?id=$blocker_id\n\n"; If you want, you could put this into messages.html.tmpl while you're here. But we should probably do that right, and make sure that we pick the right language and all. So maybe it would be best to wait for another bug. >+ $deptext .= three_columns("What ", "Old Value", "New Value"); Hmm, I don't think we should duplicate this header code, if we can avoid it. (I know it was duplicated before, but if there's a way to avoid the duplication as part of the refactoring, it would probably be good.) >+ if ($params->{dep_only}) { >+ $dep_ok = $user->can_see_bug($params->{blocker}->id) ? 1 : 0; Nit: You can just pass $params->{blocker}--can_see_bug accepts Bug objects. >@@ -442,10 +319,15 @@ sub Send { >+ if (!$params->{dep_only}) { >+ $dbh->do('UPDATE bugs SET lastdiffed = ? WHERE bug_id = ?', >+ undef, ($end, $id)); >+ $bug->{lastdiffed} = $end; Oh, hmm, that seems like a somewhat-dangerous hack. Perhaps we should just be using $end everywhere instead. >Index: Bugzilla/Bug.pm >+ type => 'dep', >+ dep_only => 1, Do we really need both "type" and "dep_only"? Should we just have a "type => 'dep_resolved'" instead? Or is it simpler to have dep_only? >+ blocker => $self, I think "blocks" might be a better name for the parameter, since that's what we use in other places. >+ bug_status => $changes->{'bug_status'}, >+ resolution => $changes->{'resolution'} }; Probably simpler just to pass $changes. Then we can start using it in other circumstances too, or people can more easily modify what is shown about these sorts of dependency changes. Generally, looks really great as a code cleanup and as a fix for this issue! :-) Everything I've mentioned above is more or less a nit, so this is good to go.

(In reply to comment #7) > Hmm. If it's really only used there, how about we just move it closer to that > block, or inside that block (if possible)? We only need it to generate new bugmails, but we don't want to call it once per user getting bugmail, so it has to be left outside the foreach loop. I can move it right before the foreach loop, though. > Also, I noticed that inside of _get_diffs you started to use $bug->lastdiffed > instead of $start--should we just be using $bug->lastdiffed everywhere instead > of $start? I use $bug->lastdiffed inside _get_diffs() because $start is not defined in the subroutine, and we already have $bug available. Using $start outside the subroutine is useful, though. It avoids having to call ->lastdiffed() all the time. > Hmm, I don't think we should duplicate this header code, if we can avoid it. Don't worry, it's going away in my next patch, in bug 466968 (I didn't upload the patch yet). > Oh, hmm, that seems like a somewhat-dangerous hack. Perhaps we should just be > using $end everywhere instead. Not sure I understand what you mean, here. > Do we really need both "type" and "dep_only"? Should we just have a "type => > 'dep_resolved'" instead? Or is it simpler to have dep_only? We need both, yes, because "type" is for the template, why dep_only is for bugmails. And when you add/remove dependencies, you don't want to trigger dep_only, despite the UI will be the same in your web browser. > I think "blocks" might be a better name for the parameter, since that's what > we use in other places. OK, I will change that. > Probably simpler just to pass $changes. Then we can start using it in other > circumstances too, or people can more easily modify what is shown about these > sorts of dependency changes. OK, I will do this change too.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Bug.pm modified Bugzilla/BugMail.pm modified Bugzilla/User.pm modified template/en/default/email/newchangedmail.txt.tmpl Committed revision 7406.