Closed Bug 1214761 Opened 9 years ago Closed 9 years ago

ADB pull fails for symlinks

Categories

(DevTools Graveyard :: WebIDE, defect)

Product:
DevTools Graveyard
Component:
WebIDE
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gerard-majax, Assigned: wdeng)

References

Details

Attachments

(2 files)

jsconsole.png
198.22 KB, image/png
Details
pull request
44 bytes, text/x-github-pull-request
jryans
: review+
gerard-majax
: feedback+
Details | Review
Attached image 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]
So I don't reproduce that from a device running matching KK base with B2G flashed, but I do reproduce with stock Android KK.
Failure is when we are pulling a symlink.
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?
Summary: ADB pull from B2G Installer broken → ADB pull fails for symlinks
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.
Or, an alternative would be to detect a symlink and just ignore them.
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.
(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 ...
(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.
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?
Flags: needinfo?(jryans)
(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
Flags: needinfo?(jryans)
I don't think we want to ignore ...
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
I'm interested but I am focusing on 2.5+ IPC Blob issues right now
(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 :)
(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. :)
Wei, can you help us?
Flags: needinfo?(wdeng)
(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.
Assignee: nobody → wdeng
Flags: needinfo?(wdeng)
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 :)
Attached file pull request — 
Do it the same as c adb client, save files symlinks link to.
Attachment #8685350 - Flags: review?(jryans)
I just tested it with some simple files, plain text and pictures, would you like to do some more tests? 

Thanks!
Flags: needinfo?(lissyx+mozillians)
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.
Attachment #8685350 - Flags: review?(jryans)
Using the same symlink test as before, this version seems to work for me.  Yay!
Comment on attachment 8685350 [details] [review]
pull request

the pull request has been updated, thanks for your review
Attachment #8685350 - Flags: review?(jryans)
Depends on: 1224160
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.
Flags: needinfo?(lissyx+mozillians)
Attachment #8685350 - Flags: feedback+
Comment on attachment 8685350 [details] [review]
pull request

Great, looks good to me! Thanks for working on this.
Attachment #8685350 - Flags: review?(jryans) → review+
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: