Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(11)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 915033002: BatteryManager.getBattery(): gracefully bail on context-detached use. (Closed)

Created:
5 years, 10 months ago by sof
Modified:
5 years, 10 months ago
CC:
blink-reviews, kinuko+fileapi, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org, nhiroki, timvolodine, tzik
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

BatteryManager.getBattery(): gracefully bail on context-detached use. Semi-blind fix for fuzzer-generated crash on accessing a BatteryManager lacking an execution context. Added a non-reproducing test to verify detached use, along with reducing test duplication for similar tests. R=jochen BUG=457010 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190031

Patch Set 1 #

Patch Set 2 : tidying up test #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -55 lines) Patch
A + LayoutTests/battery-status/detached-no-crash.html View 1 2 chunks +8 lines, -4 lines 0 comments Download
A + LayoutTests/battery-status/detached-no-crash-expected.txt View 1 chunk +3 lines, -1 line 0 comments Download
D LayoutTests/fast/dom/Window/Location/resources/set-location-after-close-new-window.html View 1 chunk +0 lines, -7 lines 0 comments Download
M LayoutTests/fast/dom/Window/Location/set-location-after-close.html View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/files/file-reader-detached-no-crash.html View 1 chunk +1 line, -1 line 0 comments Download
D LayoutTests/fast/files/resources/file-reader-detached-no-crash-new-window.html View 1 chunk +0 lines, -7 lines 0 comments Download
M LayoutTests/gamepad/gamepad-detached-no-crash.html View 1 chunk +1 line, -1 line 0 comments Download
D LayoutTests/gamepad/resources/gamepad-detached-no-crash-new-window.html View 1 chunk +0 lines, -7 lines 0 comments Download
M LayoutTests/http/tests/navigation/beacon-detached-no-crash.html View 1 chunk +1 line, -1 line 2 comments Download
D LayoutTests/http/tests/navigation/resources/beacon-detached-no-crash-new-window.html View 1 chunk +0 lines, -7 lines 0 comments Download
A + LayoutTests/resources/window-postmessage-open-close.html View 1 chunk +2 lines, -0 lines 0 comments Download
D LayoutTests/vibration/resources/vibration-detached-no-crash-new-window.html View 1 chunk +0 lines, -7 lines 0 comments Download
M LayoutTests/vibration/vibration-detached-no-crash.html View 1 chunk +1 line, -1 line 0 comments Download
D LayoutTests/webaudio/resources/scriptprocessornode-detached-no-crash-new-window.html View 1 chunk +0 lines, -7 lines 0 comments Download
M LayoutTests/webaudio/scriptprocessornode-detached-no-crash.html View 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/battery/BatteryManager.cpp View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 10 (3 generated)
sof
Please take a look. Spent too much time trying to distill a test case, but ...
5 years, 10 months ago (2015-02-12 07:16:05 UTC) #2
jochen (gone - plz use gerrit)
lgtm
5 years, 10 months ago (2015-02-12 08:26:21 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/915033002/20001
5 years, 10 months ago (2015-02-12 08:28:59 UTC) #5
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190031
5 years, 10 months ago (2015-02-12 08:32:30 UTC) #6
haraken
LGTM
5 years, 10 months ago (2015-02-12 08:34:54 UTC) #7
Michael van Ouwerkerk
Drive-by review. https://codereview.chromium.org/915033002/diff/20001/LayoutTests/http/tests/navigation/beacon-detached-no-crash.html File LayoutTests/http/tests/navigation/beacon-detached-no-crash.html (right): https://codereview.chromium.org/915033002/diff/20001/LayoutTests/http/tests/navigation/beacon-detached-no-crash.html#newcode26 LayoutTests/http/tests/navigation/beacon-detached-no-crash.html:26: w = window.open('/js-test-resources/window-postmessage-open-close.html'); Can we maybe use ...
5 years, 10 months ago (2015-02-12 14:31:32 UTC) #9
sof
5 years, 10 months ago (2015-02-12 15:51:15 UTC) #10
Message was sent while issue was closed. 
https://codereview.chromium.org/915033002/diff/20001/LayoutTests/http/tests/n...
File LayoutTests/http/tests/navigation/beacon-detached-no-crash.html (right):

https://codereview.chromium.org/915033002/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/navigation/beacon-detached-no-crash.html:26: w =
window.open('/js-test-resources/window-postmessage-open-close.html');
On 2015/02/12 14:31:32, Michael van Ouwerkerk wrote:
> Can we maybe use relative urls so that things also work when running manually?

That would be useful to also support for http/tests/, but /js-test-resources/ is
an alias (which is how js-test.js is slurped in atm for this test), so
relativising this particular url wouldn't quite work, i think.

Powered by Google App Engine
This is Rietveld 408576698