679991 - getParsedLog.php?full=1 pretends that errors have anchors which they do not

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Phil Ringnalda (:philor)]

There are links at the top of the new full logs, which look like they would be the same old links that take you down to the error part of the log, but for instance http://tbpl.allizom.org/php/getParsedLog.php?id=6011080&full=1#error1 doesn't scroll to #error1, because #error1 doesn't exist.

(For a good time, use cmd+f to find that part of the log, select from a couple of lines before to a couple of lines after, and then view selection source, to verify that it doesn't exist, on a machine with less than 8GB of RAM - I did it with a WinXP box with just 1GB, and thought I was going to have to unplug it to get it unhung.)

Comment 1

[Markus Stange [:mstange]]

Okay, where's the bug in FullLogGenerator::linkLinesWithErrors? Aren't arrays passed by reference in PHP?

Comment 2

[Phil Ringnalda (:philor)]

They are indeed, when you ask for them to be. "private function linkLinesWithErrors(&$lines, $linesWithErrors)" with an &$lines would give you $lines by reference and $linesWithErrors by value.

(Well, really it gives you both by reference, but with $linesWithErrors marked to say that if you make any changes to it, at that point it should be copied and the changes only made to the copy, but that's just an implementation detail.)

Comment 3

[Arpad Borsos [:Swatinem]]

Sorry for not catching that in the review. That is another part of php that I don’t like at all.

Comment 5

[Markus Stange [:mstange]]

Attachment: v1

Not using a reference for the array wasn't the only mistake. The other mistake was using a non-existing $i variable instead of $lineWithError.

I've also made GzipUtils::mendBrokenLines use a reference for the array in order to avoid unnecessary copying.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 559146 [details] [diff] [review]
v1

Arpad is on vacation, so maybe Robert can review this sooner.

Comment 7

[Robert Helmer [:rhelmer]]

Comment on attachment 559146 [details] [diff] [review]
v1

lgtm, I am not set up to test right now but we can verify on tbpl-dev (will auto-update after this is pushed to hg)

Comment 8

[Markus Stange [:mstange]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/41571b0daf4d

Comment 9

[Phil Ringnalda (:philor)]

Verified working on tbpl-dev (as long as you don't confuse yourself with a cached log from before it was updated).