1214761 - ADB pull fails for symlinks

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[:gerard-majax]

Attachment: jsconsole.png

+++ This bug was initially created as a clone of Bug #1210824 +++

Current nightly, ADB Helper 0.8.3, I have received reports of users still getting the RangeError as in bug 1210824 attachment 8668996 [details]

Comment 1

[:gerard-majax]

So I don't reproduce that from a device running matching KK base with B2G flashed, but I do reproduce with stock Android KK.

Comment 2

[:gerard-majax]

Failure is when we are pulling a symlink.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am able to reproduce on Z3C with B2G by adding a random symlink.  It seems specific to pulling a symlink, which maybe we have not had to deal with before?

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I spent a little time investigating this.

This does not seem like a recent regression from TCPSocket or anything like that, I just don't think we've ever supported symlinks in our pull method.

What happens is we issue a STAT command, which replies with the file size among other things.  However, for a symlink, the file size reported is the tiny size of the symlink, not the target's data size.

During the data transfer, you are never told the actual file size.  Looking at the original ADB source, they do not rely on the file size from STAT.  Instead, they just keep getting chunks and write them to an fd until they stop coming.

The pull method needs a rewrite to support this.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Or, an alternative would be to detect a symlink and just ignore them.

Comment 6

[Florent CURE]

I experienced this bug with Alexandre yesterday.
I dont know if it's relevant, but here is a bit of information about my system :
I'm using Ubuntu 14.04 LTS on an core i7-4790 with 16GB RAM.
I was using Firefox Nightly (version 44.0a1, Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0)
Just feel free to ask me for any additional information.

Comment 7

[:gerard-majax]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> I spent a little time investigating this.
> 
> This does not seem like a recent regression from TCPSocket or anything like
> that, I just don't think we've ever supported symlinks in our pull method.

And yet, this worked properly in the past ...

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre LISSY :gerard-majax from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> > I spent a little time investigating this.
> > 
> > This does not seem like a recent regression from TCPSocket or anything like
> > that, I just don't think we've ever supported symlinks in our pull method.
> 
> And yet, this worked properly in the past ...

So far, I don't believe that's true.

I just tested ADB Helper 0.8.0 with Beta 42 (so that's before any TCPSocket changes), and the symlink also fails there.

Comment 9

[:gerard-majax]

Well I cannot tell but for sure I could flash from that very same Android Kitkat release earlier, and we did query those symlinks. Maybe we skipped then at that time? Or it failed silently in another way.

When do you think you can get a fix for that, is there much work to complete on ADB Helper to fix that properly?

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre LISSY :gerard-majax from comment #9)
> Well I cannot tell but for sure I could flash from that very same Android
> Kitkat release earlier, and we did query those symlinks. Maybe we skipped
> then at that time? Or it failed silently in another way.

I am not sure what to say about this.  For the only case that I can reproduce, it's broken on both 0.8.0 and 0.8.3.

> When do you think you can get a fix for that, is there much work to complete
> on ADB Helper to fix that properly?

Because ADB pull is not used by DevTools, this is not a high priority for us.

There are several options:

* Fix pull to work for symlinks
* Expose stat API so that callers can detect symlinks and ignore them

Comment 11

[:gerard-majax]

I don't think we want to ignore ...

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am not saying it should be ignored, just that it could be quite some time before we are able to look into it as it is low priority for DevTools.

:gerard-majax, are you interested in investigating it?

The DevTools team did not actually add the pull code, it came from :wdeng originally[1], so I don't really know it in great detail.  For comparison, here is what seems like Google's pull code[2] in ADB.

So, as I said in comment 10, I think the main options are:

* We could expose a new stat() method from adb.js that B2G installer could check to get data like "is symlink" (some of this is already in ADB Helper, see `checkFileMode`[3]) and skip them
* pull could be reimplemented to use some kind of growable buffer, instead of requiring a STAT packet to return the data size that will be transferred

[1]: https://github.com/mozilla/adbhelper/commit/f0ddf8ff8dd86fa5d6a3df136795624d4d4d5468
[2]: https://android.googlesource.com/platform/system/core/+/master/adb/file_sync_client.cpp#353
[3]: https://github.com/mozilla/adbhelper/blob/master/adb.js#L377

Comment 13

[:gerard-majax]

I'm interested but I am focusing on 2.5+ IPC Blob issues right now

Comment 14

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> I don't think we want to ignore ...

I was referring to ignoring the symlink, re-reading your comment 12 I think you misunderstood and though I was referring to the bug not being fixed :).

I think we should pull the symlink as a symlink otherwise we might expose ourselves to other problems :)

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre LISSY :gerard-majax from comment #14)
> (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > I don't think we want to ignore ...
> 
> I was referring to ignoring the symlink, re-reading your comment 12 I think
> you misunderstood and though I was referring to the bug not being fixed :).
> 
> I think we should pull the symlink as a symlink otherwise we might expose
> ourselves to other problems :)

Ah, good to know!  I did think you were saying the bug at the time, so the clarification is good to have. :)

Comment 16

[:gerard-majax]

Wei, can you help us?

Comment 17

[[e-mail]]

(In reply to Alexandre LISSY :gerard-majax from comment #16)
> Wei, can you help us?

OK, I will look into it in the next week.

Comment 18

[[e-mail]]

I tried and found that, when pulling a symlink, the data received in the client is the file the symlink linked to, not the symlink file itself. So, pull a symlink as a symlink, we should modify the adb server part.


(In reply to Alexandre LISSY :gerard-majax from comment #14)
> (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > I don't think we want to ignore ...
> 
> I was referring to ignoring the symlink, re-reading your comment 12 I think
> you misunderstood and though I was referring to the bug not being fixed :).
> 
> I think we should pull the symlink as a symlink otherwise we might expose
> ourselves to other problems :)

Comment 19

[[e-mail]]

Attachment: pull request

Do it the same as c adb client, save files symlinks link to.

Comment 20

[[e-mail]]

I just tested it with some simple files, plain text and pictures, would you like to do some more tests? 

Thanks!

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8685350 [details] [review]
pull request

I added a few small nits, but overall looks good.  Waiting for a device to charge so I can test, but hoping :gerard-majax can test as well.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Using the same symlink test as before, this version seems to work for me.  Yay!

Comment 23

[[e-mail]]

Comment on attachment 8685350 [details] [review]
pull request

the pull request has been updated, thanks for your review

Comment 24

[:gerard-majax]

Comment on attachment 8685350 [details] [review]
pull request

Fixed the issue for me too: I have been able to make use of the addon on a Z3C running KK.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8685350 [details] [review]
pull request

Great, looks good to me! Thanks for working on this.

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Merged: https://github.com/mozilla/adbhelper/commit/67785f7cf78333e07c7363719015dc17c508aace

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Released in ADB Helper 0.8.6.