474234 - XPI diff tool does not work on multi item XPI

Status: VERIFIED FIXED

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

Description

[Brian King [:kinger]]

Multi item packages are explained here:

https://developer.mozilla.org/en/Multiple_Item_Packaging

In the case of an XPI containing other xpi files, the editor diff tool can not read them.

The solution of just to treat xpi files like any other compressed file (e.g. jar file).

Here is a review that has one, have a look quick before it is reviewed and expires!

https://addons.mozilla.org/en-US/editors/review/59339

Comment 1

[u278084]

Yep. This is also a problem with view contents. We also only handle jar files 1 level deep (eg. chrome/foo.jar will be extracted, but not any jar files within foo.jar), so the solution might be slightly more difficult.

Comment 2

[Joe Devietti]

Maybe this will help with testing: I have a multi-item .xpi for my extension at https://addons.mozilla.org/en-US/firefox/addon/11150.  The (public) "view the source" link on the above page displays the nested .xpi files in some binary encoding, instead of recursing into the .xpi as one would hope.

Comment 3

[Nick Nguyen [:osunick]]

Setting to P3 (nice to have) for 5.0.7 so we don't lose it.  will be P2 in 5.0.8 if we don't get to it.

Comment 4

[u278084]

I'll try to get it in for 5.0.7

Comment 5

[u278084]

Attachment: wip

Still needs some work, but great progress is being made. Started doing view contents code first because it's easier and will be pretty useful.

Comment 6

[u278084]

Attachment: screenshot

Comment 7

[Nick Nguyen [:osunick]]

Any chance this can land in 5.0.7?

Comment 8

[Nick Nguyen [:osunick]]

Bug 496387 - in rey's example I saw the same behavior but I didn't see any nested archives.  Can you investigate to see if it's the same underlying cause?

Comment 9

[u278084]

Attachment: v2

This could be the final patch. It's too late for 5.0.7 unfortunately.

Comment 10

[Wil Clouser [:clouserw]]

No problem, we'll grab it in 5.0.8.  Thanks Cesar.

Comment 11

[Rey Bango]

If this fixes the issue of the diff tool not loading files, I'd like to ask that we push it today since it will be a big help for the weekend burndown. Can we get this pushed today?

Comment 12

[Wil Clouser [:clouserw]]

Pushing a patch to production mid-cycle should only be for emergencies.  Since this is so low impact (number of multi item XPIs) and has a viable alternative (download, use command line diff) let's leave it at 5.0.8.

Comment 13

[Rey Bango]

Normally I would agree but 5.0.8 is a month away & if this patch also fixes this issue (https://bugzilla.mozilla.org/show_bug.cgi?id=496387) then I think it's worthwhile pushing now since that is a blocker for the editorial process. 
 
Can you confirm whether if it fixes the issue outlined in the bug I just mentioned?

Comment 14

[Rey Bango]

I'll start looking at command line diff tools for the editors in the interim but if this patch allows the editors to review the add-on without adding an extra step to their workflow, it would be great to get that out to them as they're helping us out by volunteering their time. I'm really hesitant about asking them to take a step backwards in the editorial process when we might have a fix available.

Comment 15

[u278084]

I agree with Wil on this. I don't think this is the cause of bug 496387. The add-ons that are effected by that bug aren't multi-item extensions. And I did't do anything special that would explain fixing it. So I will be surprised if it does fix it.

This patch is also pretty big, and touches a bit of everything. I think this should be postponed until after the burndown simply because it might introduce new bugs (ha!).

Comment 16

[Rey Bango]

Ok. If it can't be confirmed that the fix would resolve the issue, that makes sense.

Comment 17

[u278084]

Comment on attachment 385298 [details] [diff] [review]
v2

Let's see if we can get this in before the freeze this time.

Comment 18

[Fred Wenzel [:wenzel]]

Comment on attachment 385298 [details] [diff] [review]
v2

Code looks good, but I wasn't able to check it out for lack of example files ;) If you are confident it works, check it in.

The screenshot looks good too, btw.

Comment 19

[Rey Bango]

Let me know when it's on preview and I'll test it out.

Comment 20

[Wil Clouser [:clouserw]]

Can we get this committed and tested? thanks

Comment 21

[u278084]

In r48028. Thanks Wenzel.

I also set up a multi-item test extension on preview with the source publicly viewable. You can find it on https://preview.addons.mozilla.org/en-US/firefox/addon/12485/ You will have to log-in.

This is the same extension as in the screenshot.

Comment 22

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

Verified FIXED; reproduced the bug on production and verified on staging.