1521980 - moz-phab throws "Invalid Content Encoding" error with a diff containing binary files

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect, P2)

Description

[David Teller [:Yoric] - still alive but not very active]

After a ./mach vendor rust, I tried to use moz-phab to publish the change to phabricator. After 24h (no output), I killed the task.

Certainly related to bug 1498171 and Bug 1494697, not sure whether it's the same bug.

That's a pretty large problem as we regularly need to run that command.

Comment 1

[Bobby Holley (:bholley)]

It sounds like the current limit is 32MB. How big is the diff you're trying to submit?

Comment 2

[Bobby Holley (:bholley)]

(And if the answer is >= 32MB, my next question is what those dependencies are, and whether we've made a thoughtful decision about whether it makes sense to pull them in).

Comment 3

[:glob ✱]

this is most likely a duplicate of bug 1492214, however moz-phab shouldn't stall.
let's focus this bug moz-phab stalling rather than the >= 32MB patch issue.

i suspect what's happening is arc is waiting on user input, however it's hard to know without seeing the actual output.

david - can you please re-run with debugging enabled - DEBUG=1 moz-phab submit - and attach the output to this bug.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to Bobby Holley (:bholley) from comment #1)

It sounds like the current limit is 32MB. How big is the diff you're trying to submit?

6Mb

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1521980 - Payload that freezes moz-phab (part 1) - DO NOT LAND;r?glob

Please ignore r?, I need to specify a reviewer and it cannot be me.

Comment 6

[:glob ✱]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)

(In reply to Bobby Holley (:bholley) from comment #1)

It sounds like the current limit is 32MB. How big is the diff you're trying to submit?

6Mb

note that due to how phabricator's file context is implemented it submits a full copy of each file touched by a diff - see bug 1517247.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: moz-phab.log

So, this is what I get after ~15 minutes.

I can't swear that it's frozen, but I haven't seen any activity in the last ~10 minutes and both php and python are currently running at 0% CPU, so afaict, it's not doing anything.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1521980 - Payload that freezes moz-phab (part 2) - DO NOT LAND;r?glob

Please ignore r?, I need to specify and a reviewer and it cannot be me.

Depends on D17353

Comment 9

[:glob ✱]

after some more diagnostics over irc arc is indeed throwing up a prompt:

Invalid Content Encoding (Non-UTF8)
This diff includes files which are not valid UTF-8 (they contain invalid byte
sequences). You can either stop this workflow and fix these files, or
continue. If you continue, these files will be marked as binary.

You can learn more about how Phabricator handles character encodings (and how
to configure encoding settings and detect and correct encoding problems) by
reading 'User Guide: UTF-8 and Character Encoding' in the Phabricator
documentation.

    AFFECTED FILES
    third_party/rust/sha2/tests/data/sha224/test1.output.bin
    third_party/rust/sha2/tests/data/sha224/test2.output.bin
    third_party/rust/sha2/tests/data/sha224/test3.output.bin
    third_party/rust/sha2/tests/data/sha256/one_million_a.output.bin
    third_party/rust/sha2/tests/data/sha256/test1.output.bin
    third_party/rust/sha2/tests/data/sha256/test2.output.bin
    third_party/rust/sha2/tests/data/sha256/test3.output.bin
    third_party/rust/sha2/tests/data/sha384/test1.output.bin
    third_party/rust/sha2/tests/data/sha384/test2.output.bin
    third_party/rust/sha2/tests/data/sha384/test3.output.bin
    third_party/rust/sha2/tests/data/sha512/test1.output.bin
    third_party/rust/sha2/tests/data/sha512/test2.output.bin
    third_party/rust/sha2/tests/data/sha512/test3.output.bin
    third_party/rust/sha2/tests/data/sha512_224/test1.output.bin
    third_party/rust/sha2/tests/data/sha512_224/test3.output.bin
    third_party/rust/sha2/tests/data/sha512_256/test1.output.bin

    Do you want to mark these files as binary and continue? [Y/n] 

Comment 10

[:glob ✱]

there's two issues here:

• arc prompts shouldn't cause moz-phab to freeze
• binary files should be handled automatically

looking over arc's code it appears that if we connect the subprocess's STDIN to a pipe, arc will throw an exception instead of waiting for a response. look for phutil_console_require_tty in libphutil's src/console/format.php.

Comment 11

[:glob ✱]

i've split the handling of arc's prompts into bug 1522391.

Comment 12

[Bobby Holley (:bholley)]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)

(In reply to Bobby Holley (:bholley) from comment #1)

It sounds like the current limit is 32MB. How big is the diff you're trying to submit?

6Mb

Ok, that's still really big, and would be ideal to not land into mozilla-central (for repo-bloat reasons). Is it a lot of code, or are those binary test files really large? If they're large, they almost certainly don't need to be packaged as part of the crate served on crates.io, and we should fix that upstream.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

(In reply to Bobby Holley (:bholley) from comment #12)

6Mb

Ok, that's still really big, and would be ideal to not land into mozilla-central (for repo-bloat reasons). Is it a lot of code, or are those binary test files really large? If they're large, they almost certainly don't need to be packaged as part of the crate served on crates.io, and we should fix that upstream.

I was a bit surprised that a patch whose main effect was removing dependencies was so large, so I dug a bit into it.

• the total size of all files that are added/updated is 1.7Mb, included ~300kb of non-binary tests;
• roughly 1/3 of this is a file that was added as a side-effect of running ./mach vendor rust (minor version increment in a dependency of some other crate part of mozilla-central) but is unrelated to my current work;
• only a few files were actually updated, accounting for a few hundred kb, the rest were added;
• the meat of the diff file seems to be the removed files that are copied in their entirety into the diff.

I'm not sure what I can improve there.

Comment 14

[Bobby Holley (:bholley)]

I see. I obviously have no objection to removing things. I think my main ask here is to look at the things that are added, and if it's a lot of stuff, ask whether it's really necessary, or whether it could be slimmed down in some way. For example, looking at the diff, it would be nice to see if we could avoid pulling in two copies of |nom|.

Looking at the diff here, it looks like dropping the webidl crate gets rid of lalrpop, which has a pile of possibly-superfluous dependencies. Certainly happy to be rid of it.

Comment 15

[Bobby Holley (:bholley)]

Though it does seem like you were the one who added this dependency originally, and the size issue was discussed in bug 1437004 comment 65.

Comment 16

[Piotr Zalewa [:zalun]]

I'm failing to replicate the message mentioned in #9.

On a local system (Suite):
If a file is small (tried below 100kB) it comes through as a file.
If it is bigger (3.6MB) it "hangs" the mozphab with the Failed to upload binary "temp_file.bin". message. It is right after the step provided in the log (attachment 9038506 [details]):

[...]
>>> [10] (+1,383) <exec> $ HGPLAIN=1 hg cat --rev '.' --output '/tmp/a6i6s1pez4844gs0/'%p -- 'temp_file.bin'
<<< [10] (+1,545) <exec> 162,280 us
>>> [11] (+1,592) <conduit> file.allocate() <bytes = 271>
>>> [12] (+1,592) <http> http://phabricator.test/api/file.allocate
<<< [12] (+1,666) <http> 74,115 us
<<< [11] (+1,666) <conduit> 74,393 us
>>> [13] (+1,702) <http> http://phabricator.test/api/file.upload
<<< [13] (+1,723) <http> 20,021 us
Failed to upload binary "temp_file.bin".

On the Phabricator dev site: all is working fine - detected and uploaded as a binary file (tried with a a 6m file created with mkfile).

Comment 17

[David Teller [:Yoric] - still alive but not very active]

(In reply to Bobby Holley (:bholley) from comment #15)

Though it does seem like you were the one who added this dependency originally, and the size issue was discussed in bug 1437004 comment 65.

Indeed. I'm very happy that a contributor found a better way to do this, because at the time, I don't remember anyone suggesting an alternative at the time.

For example, looking at the diff, it would be nice to see if we could avoid pulling in two copies of |nom|.

Yes, this would be nice. I believe that this has just been fixed by upstream, so it's possible that me running ./mach rust vendor again today would result in a smaller diff.

Generally speaking, I agree that reducing dependency size is good, but it's also extremely time-consuming – to land previous versions of the tool, I spent weeks understanding and patching and nagging various upstreams. I'm sure that there is a better way to proceed, but I haven't found it yet.

Comment 18

[Piotr Zalewa [:zalun]]

:Yoric can you tell me what is the value of the arc_last_check in the ~/.moz-phab-config`?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

(In reply to Piotr Zalewa [:zalun] from comment #18)

:Yoric can you tell me what is the value of the arc_last_check in the ~/.moz-phab-config`?

arc_last_check = 1547722108

Comment 20

[Piotr Zalewa [:zalun]]

Thanks,

I thought it might be an old arc, but it seems to be fine.
BTW For some time already the reviewer is optional.

I'm still trying to replicate the problem and I'm wondering what could trigger the prompt.Arcanist is properly recognizing binary files for me and by default it's not asking for confirmation.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: .moz-phab-config

Attaching .moz-phab-config in case it's useful.

Comment 22

[Piotr Zalewa [:zalun]]

What OS are you using?

Comment 23

[David Teller [:Yoric] - still alive but not very active]

macOS 10.14.2

Comment 24

[Piotr Zalewa [:zalun]]

That's the same as I've got.
I think we will leave it hanging for a while and see if it'll happen again.

Comment 25

[Haik Aftandilian [:haik]]

This may belong on a new bug, but I'm running into an issue where I can't submit a diff modifying uriloader/exthandler/mac/nsOSHelperAppService.mm due to the file's encoding in the tree. See moz-phab command below. I'm hitting this on macOS 10.14.3 using mozilla-central.

1. moz-phab doesn't allow me to enter 'Y' to the arc prompt "Do you want to mark this file as binary and continue? [Y/n]" because the program exits (see error below.)

2. Even though the file has been changed to UTF-8 in the outgoing changeset, the prompt about converting to binary still appears.

3. In the Phabricator web UI, there is no way to get a side-by-side diff of uriloader/exthandler/mac/nsOSHelperAppService.mm. I suspect once the UTF8-8 version lands, future reviews will work. Changing the encoding of the new file in the UI doesn't seem to have an effect.

At present, the file is not UTF-8 in the tree:

$ file uriloader/exthandler/mac/nsOSHelperAppService.mm
uriloader/exthandler/mac/nsOSHelperAppService.mm: Objective-C source text, ISO-8859 text

Example trying to submit a review containing a change to uriloader/exthandler/mac/nsOSHelperAppService.mm:

$ moz-phab submit 457323 457324
Submitting 2 commits:
(D15626) 457324:c5fa9e093d48 Bug 1452278 - Part 2 - Limit length of MIME types and extensions received in HandlerServiceParent r?bzbarsky
(D15620) 457323:8212ab0c8998 Bug 1452278 - Part 1 - Make macOS nsOSHelperAppService::GetFromTypeAndExtension() not call OS MIME API's in content r?bzbarsky,froydnj
... (some lines omitted)
Linting...
No lint engine configured for this project.
Running unit tests...
No unit test engine is configured for this project.
Invalid Content Encoding (Non-UTF8)
This diff includes a file which is not valid UTF-8 (it has invalid byte
sequences). You can either stop this workflow and fix it, or continue. If you
continue, this file will be marked as binary.

You can learn more about how Phabricator handles character encodings (and how
to configure encoding settings and detect and correct encoding problems) by
reading 'User Guide: UTF-8 and Character Encoding' in the Phabricator
documentation.

    AFFECTED FILE
 Exception 
    uriloader/exthandler/mac/nsOSHelperAppService.mm

The program is attempting to read user input, but stdin is being piped from some other source (not a TTY).
(Run with `--trace` for a full exception trace.)
    Do you want to mark this file as binary and continue? [Y/n]
CalledProcessError: Command '['/Users/haftandilian/r/arcanist/arcanist/bin/arc', 'diff', '--base', 'arc:this', '--allow-untracked', '--no-amend', '--no-ansi', '--message-file', '/var/folders/hf/d58ybdqd5pn4c9_zlft359_c0000gn/T/tmpgpCzmH', '--message', 'Revision updated.', '--update', '15620']' returned non-zero exit status 1

Comment 26

[Haik Aftandilian [:haik]]

Attachment: ISO-8859 text encoded source file from mozilla-central nsOSHelperAppService.mm

Attachment for comment 25.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Attachment: r.diff

I experienced this issue on https://phabricator.services.mozilla.com/D28953
with the attached patch (reverting the change fixed the issue)

Comment 30

[:glob ✱]

fixed by Bug 1529175