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

VERIFIED FIXED in 5.6

Status

addons.mozilla.org Graveyard
Admin/Editor Tools
P3
major
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: jorgev, Assigned: cesar)

Tracking

unspecified
Bug Flags:
in-litmus ?

Details

(Whiteboard: [ReviewTeam], URL)

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Another example: https://addons.mozilla.org/en-US/editors/review/86895
(Reporter)

Comment 2

8 years ago
OK, this is happening to me about 50% of the time, on top of reports received from other editors.
-> major
Severity: normal → major
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.
(Reporter)

Updated

8 years ago
Duplicate of this bug: 521427
(Reporter)

Updated

8 years ago
Duplicate of this bug: 541152
I don't want to talk about it.
Assignee: nobody → clouserw
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(this is fixed in production too)
Hmm, I might have lied on this one.  Appears to still be intermittent.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(Assignee)

Comment 10

8 years ago
Since I regressed, I will take it.
Assignee: clouserw → a.sacred.line+bugzilla
Status: REOPENED → ASSIGNED
(Assignee)

Comment 11

8 years ago
Created attachment 422916 [details] [diff] [review]
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.
Attachment #422916 - Flags: review?(clouserw)
Comment on attachment 422916 [details] [diff] [review]
fix

That works for me, please commit.  Thanks cesar
Attachment #422916 - Flags: review?(clouserw) → review+
(Assignee)

Comment 13

8 years ago
Thanks. In r60986
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Keywords: push-needed
Resolution: --- → FIXED
Created attachment 423195 [details]
Post-fix screenshot
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?

Comment 16

8 years ago
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!
Keywords: push-needed
(Reporter)

Comment 17

6 years ago
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.