timvolodine@ could you take a look at this and also assign an appropriate component?

Tentatively setting to Blink>Sensor

Can you either attach a full sample file here or provide a link to one? I've tried the snippet in the bug report on both M67 stable and M68 beta and in both cases |p| is a resolved promise. All tests in http://w3c-test.org/battery-status/battery-promise.https.html and http://w3c-test.org/battery-status/battery-interface-idlharness.https.html also pass here.

Hmm, I can actually reproduce the issue if I run the excerpt from the report a few times.

This isn't strictly related to the battery status implementation, but a more general bindings issue: when something like JSON.stringify(huge Float32Array) is called, V8's GC can run and we end up hitting https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/script_promise_property_base.cc?l=83&rcl=22ad976643568040b1e99613fcc5065ce79acd5e and never resolving the promise.

yukishiino: I wonder if it makes sense to just reject the promise with a standard message in this case?

#7: That doesn't sound like expected behaviour to me. Surely either the script has already called Promise.then (in which case its handler should be kept alive by the resolver) or it intends to do so (in which case it should be keeping the v8::Promise alive).

Promise resolution shouldn't fail because a GC happened.

This reduced test case fails almost all the time here (i.e. "hello, world" is never printed), and the condition I described seemed to happen whenever there was a failure.

Do you mean the battery code's doing something wrong or is it the analysis in #7 that doesn't make sense?

Deleted: battery.html 317 bytes

battery.html 317 bytes View Download

Er, I ended up changing the test case after writing the comment. There's no "hello, world", but the div should turn green.

Yes, I think this is a bug in BatteryManager, our promise code, or both. We shouldn't reject the promise because of GC; we should do exactly what we would do if GC did not occur.

It appears to me that wrapper tracing should reach, but is not currently reaching, the resolver, at least until the promise is resolved or rejected. BatteryManager is ActiveScriptWrappable, so it does seem to be a wrapper tracing root as long as HasPendingActivity returns true. That doesn

I suspect we also need ScriptPromisePropertyBase to trace the weak persistents it holds, so that the resolvers are held alive, in a similar way to how event listeners are.

Looking more closely, the latter appears to be a non-issue because the ScriptPromisePropertyBase's "holder" is the BatteryManager itself. It appears that this is the correct (but slightly confusing use) of that class.

Fix: https://chromium-review.googlesource.com/c/chromium/src/+/1106543

included layout test times out without fix, passes with fix

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76ba6a74742801ccf97fb7e0cd804357c2c42c58

commit 76ba6a74742801ccf97fb7e0cd804357c2c42c58
Author: Jeremy Roman <jbroman@chromium.org>
Date: Wed Jun 20 10:30:20 2018

Include pending promise resolution in BatteryManager::HasPendingActivity.

This prevents the resolver from being prematurely collected.

Bug:  852751 
Change-Id: I9516c9cdb84133291008cb045108e41baa956272
Reviewed-on: https://chromium-review.googlesource.com/1106543
Commit-Queue: Raphael Kubo da Costa (CET) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568784}
[add] https://crrev.com/76ba6a74742801ccf97fb7e0cd804357c2c42c58/third_party/WebKit/LayoutTests/battery-status/no-gc-with-pending-promise.html
[modify] https://crrev.com/76ba6a74742801ccf97fb7e0cd804357c2c42c58/third_party/blink/renderer/modules/battery/battery_manager.cc

Doesn't seem like a recent regression, so I'm inclined to let it ship on schedule in M69. (Though it wouldn't be terribly risky to cherrypick into M68 if we had a good reason to do that.)

The NextAction date has arrived: 2018-06-28