539761 - Diff tool doesn't expand JAR file in some cases, even when code viewer does

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P3)

Description

[Jorge Villalobos [:jorgev] (he/him)]

To reproduce, go to https://addons.mozilla.org/en-US/editors/review/91162 and try both the code viewer and the diff tool. You'll see that the code viewer expands the chrome JAR file, while the diff tool doesn't. This seems to be a recent and frequent problem, according to the reports I've received.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Another example: https://addons.mozilla.org/en-US/editors/review/86895

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

OK, this is happening to me about 50% of the time, on top of reports received from other editors.
-> major

Comment 3

[Andrew Williamson [:eviljeff]]

I've noticed this too and its definitely a recent issue (I'm not sure which release of AMO the regression is from though).  I think it should be bumped up the priority queue as potentially dangerous code may be being missed in updates because the jar doesn't show any change.

p.s. #521427 - dupe.

Comment 6

[Wil Clouser [:clouserw]]

I don't want to talk about it.

Comment 7

[Wil Clouser [:clouserw]]

(this is fixed in production too)

Comment 8

[Wil Clouser [:clouserw]]

Hmm, I might have lied on this one.  Appears to still be intermittent.

Comment 9

[Wil Clouser [:clouserw]]

Alright, this is caused by Cesar's patch in r56873 and wenzel's patch in 58938.  There are a few issues:

1) $isJar in _unwrapDiff() will never be true because _computeArchiveHash() doesn't extract jar files (the by_preg flag doesn't match .jar or .xpi).

2) _computeArchiveHash() is clearning out $v['content'] in an effort to save memory (yay) but _unwrapDiff(), after $isJar matches, is trying to write ['content'] to disk so it can extract it.

Cesar/wenzel, do you have time to fix this before monday?

Comment 10

[u278084]

Since I regressed, I will take it.

Comment 11

[u278084]

Attachment: fix

Here is a fix. The problem was closer to your second point. The v['content'] was being destroyed, but we needed it to be present for jar/xpi files so we can extract it to disk.

What puzzles me is that this was only occurring, as stated in comment 2, 50% of the time.

We do match on $isJar because we join the matched files with the return value of listContents(), which lists all the files in the archive.

I made some quick adjustments that I hope is ok. I also made it possible for us to properly expand .JAR files. As well, the view page will now not be double-spaced. Which I'm pretty sure is a bug that has never been filed.

Comment 12

[Wil Clouser [:clouserw]]

Comment on attachment 422916 [details] [diff] [review]
fix

That works for me, please commit.  Thanks cesar

Comment 13

[u278084]

Thanks. In r60986

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Post-fix screenshot

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/files/diff/75679/; I could easily reproduce it on production, and it worked fine on staging.

Comment 16

[Oliver Saier]

Could we push this ASAP? 3.6 is out and we're having a crapload of alleged instal.rdf updates in the review queue. Thanks!

Comment 17

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...