1035437 - Add an adb wakelock

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[Dave Hylands [:dhylands]]

Add an adb wakelock, which will cause the adb connection to stay up as long as the wakelock is held.

This would be useful, during a build for say building the backup-xxx directory.

Currently, on my flame, If I wake my flame up and start a build which needs to create backup-flame, the screen will timeout and break the adb connection before the pull of /system completes.

Once the adb wakelock exists, then the build can do:

> adb shell echo adb > /sys/power/wake_lock

and then when the pull of /system is finished, it can do:

> adb shell echo adb > /sys/power/wake_unlock

You can cat the above files to query held/unheld wakelocks.

Comment 1

[Dave Hylands [:dhylands]]

having run-gdb.sh acquire and release the adb wakelock would be another good place to use this (since dropping the adb session kills gdb as well)

Comment 2

[Tanmaya Mishra]

Hi! I want to try helping out with this. Haven't collaborated before though. How should I start? Thank you!

Comment 3

[Dave Hylands [:dhylands]]

(In reply to Tanmaya Mishra from comment #2)
> Hi! I want to try helping out with this. Haven't collaborated before though.
> How should I start? Thank you!

The first thing would be to build a Firefox OS image so that you can test the feature.

Do you have a FirefoxOS phone? Or would you be using the emulator?

https://developer.mozilla.org/en-US/Firefox_OS/Building_and_installing_Firefox_OS

When you get to the step where you need to configure, then this for the emulator (ignore the > at the beginning of the line - that just allows me to highlight the line in bugzilla)

> ./config.sh emulator

For a real phone, it will depend on the phone in question.

The code that needs to check forthe wakelock is part of the AdbController which can be found in the gecko/b2g/chrome/content/adb.js file in your B2G source tree.


Right around here: http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools/adb.js#202 is where the check for the wakelock would need to go.

Querying the wakelock will involve reading the /sys/power/wake_lock file and parsing the contents.

Comment 4

[Dave Hylands [:dhylands]]

Oh yeah - in order to test this properly, you'll need to create a user build, which will involve creating a .userconfig file in the root of your B2G directory tree.

See: https://developer.mozilla.org/en-US/Firefox_OS/Customization_with_the_.userconfig_file

IN particular, you'll want to put the line:

VARIANT=user

into your .userconfig file (without that line, you'll build an eng build, which has adb enabled all of the time - so you won't be able to test the feature properly).

Comment 5

[Tanmaya Mishra]

I don't have a Firefox phone so I suppose the emulator will be the way to go for me. I do have a nexus 5 with a Firefox os build running on it thanks to the wonderful devs at XDA. I will have to check how the developer has built it though and hopefully learn it. And thanks for the links!

Comment 6

[Dave Hylands [:dhylands]]

I assigned the bug to you. If you change your mind, or otherwise can't complete the bug, please let me know and I'll set it back to nobody.

Comment 7

[Tanmaya Mishra]

I went through the build documentation and at my download speed, I don't expect it to take less than 2 days. Until then I have a few questions. I went through adb.js. I don't fully understand it but reading the wake_lock file for "adb" (considering the example you gave in comment 0) seems straightforward. But after determining if its there, what should I do with it? According to what I have understood, I should ensure  enableAdb remains true but I am not sure. Thanks, by the way, for helping me out!

Comment 8

[Dave Hylands [:dhylands]]

(In reply to Tanmaya Mishra from comment #7)
> I went through the build documentation and at my download speed, I don't
> expect it to take less than 2 days. Until then I have a few questions. I
> went through adb.js. I don't fully understand it but reading the wake_lock
> file for "adb" (considering the example you gave in comment 0) seems
> straightforward. But after determining if its there, what should I do with
> it? According to what I have understood, I should ensure  enableAdb remains
> true but I am not sure. Thanks, by the way, for helping me out!

More or less. The code you add should look something like this:

> read line from /sys/power/wake_lock
> split line by spaces
> if (adb is one of the words) {
>   enableAdb = true;
>   useDisableAdbTimer = false;
>   if (this.DEBUG) {
>     this.debug("Keeping ADB enabled due to adb wakelock");
>   }
> }

Note that the contents of the wakelock file may contain multiple space separated wakelocks. You can see this by doing something like:

> 2344 >adb shell
> root@flame:/ # echo foo > /sys/power/wake_lock                                 
> root@flame:/ # echo adb > /sys/power/wake_lock                                 
> root@flame:/ # echo bar > /sys/power/wake_lock                                 
> root@flame:/ # cat /sys/power/wake_lock                                        
> adb bar foo

TIP: In the future, if you need to create a second tree, you can speed things up quite a bit by doing:

> git clone https://github.com/mozilla-b2g/B2G.git B2G-new
> rsync -ap B2G-old/.repo/ B2G-new/.repo/

and then doing the ./config.sh

Comment 9

[Tanmaya Mishra]

Hi! I finally got it to build as I was running into a few problems. So, now I have two questions. How do I check whether the build is a user build or not and how do I test my changes? Also, if it works where do I upload the changes? Thank you.

Comment 10

[Dave Hylands [:dhylands]]

If you cd into the objdir-gecko/config directory and do:

> grep ENABLE_MARIONETTE *


then you'll see:

> autoconf.mk:ENABLE_MARIONETTE = 1

in the output for userdebug and eng builds, and you'll see:

> emptyvars.mk:ENABLE_MARIONETTE =

if its a user build.

To test: fire up the emulator. When the screen is unlocked you should be able to run

> adb devices

from another terminal window and see the emulator device. If the screen is locked (you can press the power button briefly to toggle between locked and unlocked) then you shouldn't see the emulator.

When the screen is unlocked, do: 

> adb shell
> echo adb > /sys/power/wake_lock

and then lock the screen. The emulator should now show up in the adb devices ouput, even though the screen is locked.

Finally, do:

> adb shell
> echo adb > /sys/power/wake_unlock

and the when you lock the screen you should see that adb is not present when the emulator is locked.

You'll need to attach your patch to the bug and get it reviewed. I'll review your changes, and in this case, I can upload your changes (you won't have sufficient priviledge yet).

This page: https://developer.mozilla.org/en/docs/Introduction documents the overall process.

I'll also test your changes on a real device, to verify the correct behaviour.

If you're using git for your gecko tree, then I would create your patch by following these steps:

> git checkout -b bug-1035437
> git add files-that-you-edited
> git commit
> git format-patch -U8 HEAD~

and then attach the generated patch to the bug. Please ensure that you've configured git with your email address and name.

The "Your Identity" section from this page: http://git-scm.com/book/en/Getting-Started-First-Time-Git-Setup

Comment 11

[Tanmaya Mishra]

Hi! Tried testing but I am running into problems. 

1) I first checked if its a user build or not with the command you suggested
> grep ENABLE_MARIONETTE *
Its output was 
> emptyvars.mk:ENABLE_MARIONETTE =
Since its a user build, I guess that's why
 
> adb shell
 
provides me with only a normal unprivileged shell. So it prevents me from writing to /sys/power/wake_lock. Will a userdebug build help?

2) I think I have landed upon a bug in the way adb interacts with the emulator. I tried running the following commands without implementing my changes. 
 
> adb devices 
When I run this for the first time after I boot up and unlock the screen, it shows
>emulator-5554 device
This continously remains so even after I lock the device and run it again. No change

Then if I kill the adb server with 
> adb kill-server 
and then restart the adb server with
> adb devices
I now get
> emulator-5554 offline
Which remains unchanged if I run it whether the device is unlocked or locked.

Should I file a bug? Thanks a lot!

Comment 12

[Tanmaya Mishra]

And yeah. Forgot to mention this. To check if there was something wrong with my adb I ran a Firefox os build for my Nexus 5 I got off XDA and connected to it. Adb works fine in that case. The command
> adb shell
connects properly with my nexus 5 and presents a shell for me to work with, when it is unlocked and disconnects automatically a few seconds after the screen gets locked.

Comment 13

[Dave Hylands [:dhylands]]

It's quite possible that the emulator behaviour with adb is slightly different from that of a real phone.

I'm on PTO (paid-time-off) Mon-Wed, but I'll see if I can confirm your behaviour.

You might be able to do:

> adb root

then followed by adb shell in order to get a root shell. If not, we can get a root shell another way, but if the lock/unlock behaviour doesn't work then there isn't much point.

There is also a way you could test on your Nexus 5 without having to build it, although there is always the risk of winding up with a phone that doesn't boot, or if there is a bug in your code, you might not be able to get back into adb.

You would still be able to recover by reflashing the XDA image, but you'd probably loose your user data, so you need to consider carefully whether you want to do that or not.

If you attach the patch to the bug, I can test it on my flame.

Comment 14

[Tanmaya Mishra]

Attachment: 0001-bug-1035437-Changes-to-adb.js-to-detect-wakelock.patch

Comment 15

[Tanmaya Mishra]

Very sorry, I didn't know what PTO stood for. I have attached a patch. Please tell me if anything is wrong or needs to be corrected. Thank you.

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 8487910 [details] [diff] [review]
0001-bug-1035437-Changes-to-adb.js-to-detect-wakelock.patch

Review of attachment 8487910 [details] [diff] [review]:
-----------------------------------------------------------------

The patch should have been created from within the gecko directory (not the B2G directory). So please recreate the patch relative to the gecko directory (I'm not quite sure how you managed to create it relative to the B2G directory using "git format-patch" since B2G and gecko are different repositories).

::: gecko/b2g/chrome/content/devtools/adb.js
@@ +199,5 @@
>        // This means that the pref doesn't exist. Which is fine. We just leave
>        // enableAdb alone.
>      }
> +
> +    // Now to check for any wakelock present to prevent adb from disconnecting 

nit: Remove trailing whitespace

@@ +203,5 @@
> +    // Now to check for any wakelock present to prevent adb from disconnecting 
> +    // on sleep
> +    let lockFile = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsIFile);
> +    let foStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> +    let coStream = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(Ci.nsIConverterInputStream);

Why the converter? The file is ASCII, not UTF8.

I'd be inclined to use scriptableinputstream like this:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1422

(that code strips the trailing newline as well).

@@ +212,5 @@
> +      foStream.init(lockFile,-1,0,0);
> +      coStream.init(foStream, "UTF-8", 0, 0);
> +      coStream.readString(-1,str);
> +      let wakeLockContents = str.value;
> +      let wakeLockList = wakeLockContents.split(" ");

When I tested this it didn't work. It seems that wakeLockContents ends in a newline character, which you need to strip off before doing the split.

When I tested, the contents of /sys/power/wake_lock was the following in hex: 61 64 62 0a which is adb followed by a newline.

@@ +221,5 @@
> +          this.debug("Keeping ADB enabled as adb wakelock is present");
> +        }
> +        coStream.close();
> +        foStream.close();
> +      }

for debug purposes, it would be useful to have an else here and put something like:

        if (this.DEBUG) {
          this.debug("No adb wakelock present");
        }

@@ +227,5 @@
> +      if (this.DEBUG) {
> +        this.debug("Wake_lock file not found.");
> +      }
> +    }
> +    //Finished checking

nit: I don't think we need the comment here.

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 8487910 [details] [diff] [review]
0001-bug-1035437-Changes-to-adb.js-to-detect-wakelock.patch

Review of attachment 8487910 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag. Please upload an updated patch and re-request review. Thanks

Comment 18

[Tanmaya Mishra]

Attachment: 0001-bug-1034537-adb.js-chnages-for-wakelock.patch

I assumed wrongly that its UTF-8. I have updated the patch with reference to the original adb.js file. I also moved foStream.close() and sStream.close() to the outer if block. Hopefully this works. Thanks again!

Comment 19

[Dave Hylands [:dhylands]]

Comment on attachment 8488712 [details] [diff] [review]
0001-bug-1034537-adb.js-chnages-for-wakelock.patch

Review of attachment 8488712 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't address all of the comments, and it turns out that nsIScriptableInputStream is unsuitable for use with /sys files (these are psuedo-files).

Please obsolete the older patches when uploading a new one (either, check the appropriate checbox, or click on Details for the patch, then click edit details, and then you can click on the obsolete checkbox.

::: b2g/chrome/content/devtools/adb.js
@@ +198,5 @@
>      } catch (e) {
>        // This means that the pref doesn't exist. Which is fine. We just leave
>        // enableAdb alone.
>      }
> +    // Now to check for any wakelock present to prevent adb from disconnecting 

nit: remove trailing whitespace (mentioned in original review and not fixed)

@@ +199,5 @@
>        // This means that the pref doesn't exist. Which is fine. We just leave
>        // enableAdb alone.
>      }
> +    // Now to check for any wakelock present to prevent adb from disconnecting 
> +    // on sleep

nit: change the "on sleep" to "when the phone is locked."

@@ +202,5 @@
> +    // Now to check for any wakelock present to prevent adb from disconnecting 
> +    // on sleep
> +    let lockFile = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsIFile);
> +    let foStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> +    let sStream = Cc["@mozilla.org/scriptableinputstream;1"].createInstance(Ci.nsIScriptableInputStream);

Bah - So it turns out that the way scriptableInputStream works we can't use it to read /sys/ files.

This is because the file size is always reported as 4096 bytes and if it reads less than what the file size says, then it considers it to be an error.

So I had to switch back to the original way that you coded it.

@@ +205,5 @@
> +    let foStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> +    let sStream = Cc["@mozilla.org/scriptableinputstream;1"].createInstance(Ci.nsIScriptableInputStream);
> +    lockFile.initWithPath('/sys/power/wake_lock');
> +    if(lockFile.exists())
> +    {

nit: open curly brace should go at end of if line. See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +206,5 @@
> +    let sStream = Cc["@mozilla.org/scriptableinputstream;1"].createInstance(Ci.nsIScriptableInputStream);
> +    lockFile.initWithPath('/sys/power/wake_lock');
> +    if(lockFile.exists())
> +    {
> +      foStream.init(lockFile,-1,0,0);

nit: spaces after commas.

Please add: 
Components.utils.import("resource://gre/modules/FileUtils.jsm");
 to the top of the adb.js file (after the use strict)
and use FileUtils.MODE_RDONLY instead of -1

@@ +208,5 @@
> +    if(lockFile.exists())
> +    {
> +      foStream.init(lockFile,-1,0,0);
> +      sStream.init(foStream);
> +      let wakeLockContents = sStream.read(sStream.available());

You're still missing the removal of the newline.

I wound up doing:

    let lockFile = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsIFile);
    let foStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
    let coStream = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(Ci.nsIConverterInputStream);
    lockFile.initWithPath('/sys/power/wake_lock');
    if(lockFile.exists())
    {
      foStream.init(lockFile, FileUtils.MODE_RDONLY, 0, 0);
      coStream.init(foStream, "UTF-8", 0, 0);
      let str = {};
      coStream.readString(-1, str);
      coStream.close();
      foStream.close();

      let wakeLockContents = str.value.replace(/\n/, "");
      let wakeLockList = wakeLockContents.split(" ");

@@ +211,5 @@
> +      sStream.init(foStream);
> +      let wakeLockContents = sStream.read(sStream.available());
> +      let wakeLockList = wakeLockContents.split(" ");
> +      if (wakeLockList.indexOf("adb") >= 0) {
> +        enableAdb=true;

nit: add spaces around =

@@ +218,5 @@
> +          this.debug("Keeping ADB enabled as adb wakelock is present");
> +        }
> +      } else {
> +        if (this.DEBUG) {
> +          this.debug("Adb wakelock not found.")

nit: capitalize ADB

Missing semi-colon at the end of the line.

@@ +222,5 @@
> +          this.debug("Adb wakelock not found.")
> +        }
> +      }
> +      sStream.close();
> +      foStream.close();

I'd recommend moving the 2 close statements to be immediately after the read.

Comment 20

[Tanmaya Mishra]

I was reading through some documentation on MDN before I started making changes according to your recommendations, wouldnt OS.File be be a better method to read? I read that it's meant to replace the nsIFile related APIs and is faster too. Would this work?

> Components.utils.import("resource://gre/modules/osfile.jsm");
> wakeLockContents = OS.File.read(path, { encoding: "ascii" } ); 
> wakeLockList = wakeLockContents.split(" ");

This feels much simpler and straightforward than the nsIFile based method. Since I am decoding using ASCII I think the newline problem should also not occur and I won't have to write code to strip it out. Or should I stick to the original method?

Secondly, should I use a TextDecoder myself or can I simple use it in the above manner? As in should I use it like this instead?

> Components.utils.import("resource://gre/modules/osfile.jsm");
> let decoder = new TextDecoder("ascii");
> let wakeLockContents = OS.File.read("file.txt");
> wakeLockContents = wakeLockContents.then(
>   function onSuccess(array) {
>     return decoder.decode(array);
>   }
> );
> wakeLockList = wakeLockContents.split(" ");

Also  where do I put the import line? At the start of the file before this line? 
> let AdbController = {

Lastly, thank you for taking the time out to check my code. Must say, you seriously have a lot of patience!

Comment 21

[[:fabrice] Fabrice Desré]

OS.File has some memory usage issues because it's using a chrome worker. So I would rather stick to nsIFile even if that makes for code that is not that pretty.

Comment 22

[Tanmaya Mishra]

Attachment: 0001-bug-1035437-Changes-for-adb.js-to-detect-wakelock.patch

Please review this proposed patch. I think I have incorporated all your recommended changes. Thank you.

Comment 23

[Dave Hylands [:dhylands]]

Comment on attachment 8489395 [details] [diff] [review]
0001-bug-1035437-Changes-for-adb.js-to-detect-wakelock.patch

Review of attachment 8489395 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent. Everything seems to be working fine.

I'd like to have fabrice give it a quick look over, since he's more familiar with the JS side of things than I am.

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 8489395 [details] [diff] [review]
0001-bug-1035437-Changes-for-adb.js-to-detect-wakelock.patch

Review of attachment 8489395 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed (also check that everything fits in 80 columns). thanks!

::: b2g/chrome/content/devtools/adb.js
@@ +204,5 @@
> +    let foStream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> +    let coStream = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(Ci.nsIConverterInputStream);
> +    let str = {};
> +    lockFile.initWithPath('/sys/power/wake_lock');
> +    if(lockFile.exists()) {

nit: move the creation of foStream and coStream here.

Comment 25

[Dave Hylands [:dhylands]]

Tanmaya, are you planning on following through with this to get it landed?

Comment 26

[Tanmaya Mishra]

Sorry, I had a few college assignments to complete and couldn't get back to this. I created the patch where I moved the creation of foStream, coStream and str so that they will be created only after checking whether the wake_lock file exists. The problem is, Fabrice had asked me to check whether everything fits in 80 columns, it doesn't, so am unsure as to what to do now. Do I obsolete the previous patch and upload this one?

Comment 27

[Dave Hylands [:dhylands]]

No problem. I just wanted to make sure if wasn't forgotten.

So yes, you should obsolete the previous patch and upload the new one. Since you don't have permissions to land directly, somebody else will need to land your patch for you.

You should reformat the patch to keep the lines under 80 columns:
See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Line_Length

The normal procedure is that you would do a try run, and mark the bug as checkin-needed, and then one of the sheriffs will land the patch.

In this case, I don't think that a try run will be beneficial, so once everything is done I'll just grab the patch from the bug and take care of landing it.

Comment 28

[Tanmaya Mishra]

Attachment: 0001-bug-1035437-Changes-to-adb.js.patch

Here is the one with the required changes. Please tell me if I am wrong. I have attached you as the reviewer. Is there any way I can attach Fabrice as a reviewer too?

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 8494648 [details] [diff] [review]
0001-bug-1035437-Changes-to-adb.js.patch

Review of attachment 8494648 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

For future reference, when creating multiple versions of a patch, you should try to leave the patch description the same, and just add a v2, v3, etc to the end (makes it easier for the reviewers to do interdiff).

No need to get fabrice to re-review. I'll go ahead and land this.

Comment 30

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

Dave, did you intend to land this patch?

Comment 31

[Dave Hylands [:dhylands]]

(In reply to J. Ryan Stinnett [:jryans] from comment #30)
> Dave, did you intend to land this patch?

I do - I've just been totally swamped.

Comment 32

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f3dcc4f9ce85

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f3dcc4f9ce85