New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 845877 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 842862
issue 845508
issue v8:8507

Blocking:
issue v8:6792



Sign in to add a comment

Write Protection Issues on Android

Project Member Reported by cbruni@chromium.org, May 23 2018

Issue description

This needs more investigation to figure out the root cause of various issue / regressions on android.
 

Comment 1 by cbruni@chromium.org, May 23 2018

Blockedon: 845508

Comment 2 by cbruni@chromium.org, May 23 2018

Blockedon: 842862

Comment 3 by cbruni@chromium.org, May 23 2018

Summary: Write Protection Issues on Android (was: Write Protection Isses on Android)
Project Member

Comment 4 by bugdroid1@chromium.org, May 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/45fa14f061b6bd869201facc956e01b57430799e

commit 45fa14f061b6bd869201facc956e01b57430799e
Author: Camillo Bruni <cbruni@chromium.org>
Date: Wed May 23 14:01:22 2018

Revert "Flush ICache on startup deserialization after marking memory executable"

This reverts commit 05bcb12e554ea86e6bb03aef9db3c2fe220296ed.

Reason for revert: Causes isolate startup regressions ( https://crbug.com/845508 )

Original change's description:
> Flush ICache on startup deserialization after marking memory executable
>
> Tentative fix for Android invoke crashers with write protection code
> enabled.
>
> Bug: chromium:842862
> Change-Id: If238b25b239b50c597f3745aa683f564a717434f
> Reviewed-on: https://chromium-review.googlesource.com/1061513
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
> Commit-Queue: Camillo Bruni <cbruni@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#53209}

TBR=mstarzinger@chromium.org,cbruni@chromium.org,jgruber@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:842862,  chromium:845508 ,  chromium:845877 
Change-Id: Iff002e1ac75aca48c696053dddf1b413f372629e
Reviewed-on: https://chromium-review.googlesource.com/1068048
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53305}
[modify] https://crrev.com/45fa14f061b6bd869201facc956e01b57430799e/src/isolate.cc
[modify] https://crrev.com/45fa14f061b6bd869201facc956e01b57430799e/src/snapshot/startup-deserializer.cc
[modify] https://crrev.com/45fa14f061b6bd869201facc956e01b57430799e/src/snapshot/startup-deserializer.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 4 2018

Labels: merge-merged-6.8
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/1262d17586f35fb8991c9bfa90893af3d427a8df

commit 1262d17586f35fb8991c9bfa90893af3d427a8df
Author: Yang Guo <yangguo@chromium.org>
Date: Mon Jun 04 07:55:23 2018

Merged: Revert "Flush ICache on startup deserialization after marking memory executable"

Revision: 45fa14f061b6bd869201facc956e01b57430799e

BUG=chromium:842862, chromium:845508 , chromium:845877 , chromium:845219 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
TBR=cbruni@chromium.org

Change-Id: I4c140554b96038de6b34c572a6a6f307300bf557
Reviewed-on: https://chromium-review.googlesource.com/1084477
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.8@{#9}
Cr-Branched-From: 44d7d7d6b1041b57644400a00cb3fee35f6c51b2-refs/heads/6.8.275@{#1}
Cr-Branched-From: 5754f66f75136dc17b4c63fec84f31dfdb89186e-refs/heads/master@{#53286}
[modify] https://crrev.com/1262d17586f35fb8991c9bfa90893af3d427a8df/src/isolate.cc
[modify] https://crrev.com/1262d17586f35fb8991c9bfa90893af3d427a8df/src/snapshot/startup-deserializer.cc
[modify] https://crrev.com/1262d17586f35fb8991c9bfa90893af3d427a8df/src/snapshot/startup-deserializer.h

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 22

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/f2d39d190ceb33db86e97b4cee81f3d8d15eff28

commit f2d39d190ceb33db86e97b4cee81f3d8d15eff28
Author: Michael Starzinger <mstarzinger@chromium.org>
Date: Thu Nov 22 14:09:58 2018

[arm64] Enable write-protected code pages.

R=ulan@chromium.org
BUG= chromium:845877 

Change-Id: If8a9e2f107bb775494b1d4a83c3fb045bb846b10
Reviewed-on: https://chromium-review.googlesource.com/c/1347482
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57740}
[modify] https://crrev.com/f2d39d190ceb33db86e97b4cee81f3d8d15eff28/src/flag-definitions.h

Blockedon: v8:8507
Blocking: v8:6792
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 27

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/64d373e51ec6a65794dcca453c3066366dd09e5d

commit 64d373e51ec6a65794dcca453c3066366dd09e5d
Author: Michael Starzinger <mstarzinger@chromium.org>
Date: Tue Nov 27 12:16:43 2018

[heap] Fix i-cache flushing operation order.

This unifies the order of i-cache flushing and permission changing
throughout V8. According to cctest/test-icache flushing after the
permission change is not robust on some ARM32 and ARM64 devices.

There have been observed failures of {TestFlushICacheOfExecutable} on
some devices. So far there haven't been any observed failures of the
corresponding {TestFlushICacheOfWritable} test.

Also the order of flushing before the permission change is the natural
order in which the GC currently performs operations. Until we see
concrete data substantiating the opposite, the following is the
supported and intended order throughout V8:

  exec -> perm(RW) -> patch -> flush -> perm(RX) -> exec

This CL tries to establish said order throughout the codebase.

R=ulan@chromium.org
TEST=cctest/test-icache
BUG= v8:8507 , chromium:845877 

Change-Id: Ic945082e643aa2d142d222a7913a99816aff4644
Reviewed-on: https://chromium-review.googlesource.com/c/1351025
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57869}
[modify] https://crrev.com/64d373e51ec6a65794dcca453c3066366dd09e5d/src/heap/factory.cc
[modify] https://crrev.com/64d373e51ec6a65794dcca453c3066366dd09e5d/src/objects/code.h
[modify] https://crrev.com/64d373e51ec6a65794dcca453c3066366dd09e5d/test/cctest/test-icache.cc
[modify] https://crrev.com/64d373e51ec6a65794dcca453c3066366dd09e5d/test/common/assembler-tester.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 9

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/56bed77bd5ecba65847f373608c7918d404a1105

commit 56bed77bd5ecba65847f373608c7918d404a1105
Author: Michael Starzinger <mstarzinger@chromium.org>
Date: Wed Jan 09 11:18:15 2019

[arm] Enable write-protected code pages.

R=hablich@chromium.org
BUG= chromium:845877 

Change-Id: Ia5ede7b5aaa4d5937160b1e0733132c47afeb712
Reviewed-on: https://chromium-review.googlesource.com/c/1400407
Reviewed-by: Michael Hablich <hablich@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58658}
[modify] https://crrev.com/56bed77bd5ecba65847f373608c7918d404a1105/src/flag-definitions.h

Comment 11 by mstarzinger@chromium.org, Jan 17 (6 days ago)

Labels: -Pri-3 Pri-2
Quick status update: We have re-enabled write protected code pages on both ARM32 and ARM64 (both also on Android) by now. For ARM64 crash reports look good for Canary and Dev. For ARM32 I'd like to wait for another Dev release to be sure before closing this bug. Otherwise the work here should be done. 

Comment 12 by mstarzinger@google.com, Jan 21 (2 days ago)

Status: Fixed (was: Assigned)
No relevant crashers on the last Dev channel update from three days ago either. Considering this fixed.

Sign in to add a comment