New issue
Advanced search Search tips

Issue 820620 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

ServiceWorkerHandleTest.DispatchExtendableMessageEvent_Fail and 1 other(s) in content_unittests failing on chromium.memory/Linux CFI

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Mar 9 2018

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of hongchan@google.com

ServiceWorkerHandleTest.DispatchExtendableMessageEvent_Fail and 1 other(s) in content_unittests failing on chromium.memory/Linux CFI

Builders failed on: 
- Linux CFI: 
  https://build.chromium.org/p/chromium.memory/builders/Linux%20CFI

failures:
ServiceWorkerHandleTest.DispatchExtendableMessageEvent_FromClient
ServiceWorkerHandleTest.DispatchExtendableMessageEvent_Fail

 
Cc: -hongchan@google.com hongchan@chromium.org
Components: Blink>ServiceWorker
The offending revision is: https://chromium.googlesource.com/chromium/src/+/b2f46cdb9e38a388a94e2cb7d4b07d93a140f6ce

It was too late to revert all things after this CL landed.

Comment 2 by mek@chromium.org, Mar 9 2018

Copying my comment from the CL:

This seems to be because of a bad cast in TestWebContents::GetMainFrame. That method assumes that the RenderFrameHost is a TestRenderFrameHost, but without a RenderViewHostTestEnabler instance in your test harness that is not the case (no idea why the TestWebContents documentation doesn't say that it requires a RenderViewHostTestEnabler or similar...)
mek@

Is it reasonable to disable these test and deal with the failure later?

Comment 4 by mek@chromium.org, Mar 9 2018

Seems reasonable to me, yes. It's a bit of a weird case since the test aren't failing so much, but they're just failing the control flow integrity checks (and the failure is arguably not in the test, but in the test framework). Not sure if there is an easy way to disable them just for CFI builds.

There also seems to be a blacklist for CFI checks in //src/tools/cfi/blacklist.txt, but no idea when it would be appropriate to add something there.

So short-term to get the tree green it seems reasonable enough to me to just disable these tests.

Comment 5 by mek@chromium.org, Mar 9 2018

(btw https://www.chromium.org/developers/testing/control-flow-integrity seems to have instructions for doing a CFI build, which might help whoever ends up trying to fix this to make sure their fix worked).
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 9 2018

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

commit fe5faf93b28bbcc65a504a985603ed8085995803
Author: Hongchan Choi <hongchan@chromium.org>
Date: Fri Mar 09 23:57:37 2018

Disable ServiceWorkerHandleTest.DispatchExtendableMessageEvent_*

Bug:  820620 
Change-Id: I5f33231f7b7c8495bae2b3faf04085bfa8152ab5
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/957710
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542286}
[modify] https://crrev.com/fe5faf93b28bbcc65a504a985603ed8085995803/content/browser/service_worker/service_worker_handle_unittest.cc

Comment 7 by leon....@intel.com, Mar 10 2018

Owner: leon....@intel.com
Status: Started (was: Available)

Comment 8 by tapted@chromium.org, Mar 12 2018

 Issue 820579  has been merged into this issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 13 2018

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

commit 733a5fa4ef949b7e3357122ffcd58c17eb172c68
Author: Han Leon <leon.han@intel.com>
Date: Tue Mar 13 03:16:43 2018

[ServiceWorker] Prepare a RenderViewHostTestEnabler to create Test* instead of real one

This is to fix the 2 unit tests on CFI builds:
  ServiceWorkerHandleTest.DispatchExtendableMessageEvent_FromClient
  ServiceWorkerHandleTest.DispatchExtendableMessageEvent_Fail

BUG= 820620 , 772713 ,813749

Change-Id: Iaf4a6f4a464fe97a44f63bf2ca5a1a2e04400b9b
Reviewed-on: https://chromium-review.googlesource.com/958126
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Han Leon <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#542712}
[modify] https://crrev.com/733a5fa4ef949b7e3357122ffcd58c17eb172c68/content/browser/service_worker/service_worker_handle_unittest.cc

Labels: -Sheriff-Chromium

Comment 11 by leon....@intel.com, Mar 14 2018

Status: Fixed (was: Started)
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 22 2018

Labels: merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b

commit f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Mar 22 03:55:31 2018

[M66] service worker: Jumbo + RenderViewHostTestEnabler fixes.

This merges the following CLs from bug 813749. The net changes are:
* Fix Jumbo build by renaming PostMessage to PostMessageToServiceWorker
* Fix unit tests by using RenderViewHostTestEnabler

[ServiceWorker] Fix jumbo+Win compilation error related to 'PostMessage'

PostMessage is a macro in Windows and depending on whether Windows
headers have been included or not, PostMessage will either be treated as
PostMessage or PostMessageW.

However, our previous CL
https://chromium-review.googlesource.com/c/chromium/src/+/933867
introduced a method just named as 'PostMessage', this caused some
compile/link errors on Windows platform.

This CL renames the method to 'PostMessageToServiceWorker' to fix this.

BUG= 772713 ,813749
TBR=kinuko@chromium.org for mechanical renaming in the mojom file

Change-Id: I16241052e3deb8ba4a5fbb777037eca9904c6279
Reviewed-on: https://chromium-review.googlesource.com/959688
Commit-Queue: Han Leon <leon.han@intel.com>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542741}
(cherry picked from commit 29f8841c033d4293a362d9123fefc880d7844bf2)

[ServiceWorker] Prepare a RenderViewHostTestEnabler to create Test* instead of real one

This is to fix the 2 unit tests on CFI builds:
  ServiceWorkerHandleTest.DispatchExtendableMessageEvent_FromClient
  ServiceWorkerHandleTest.DispatchExtendableMessageEvent_Fail

BUG= 820620 , 772713 ,813749

Change-Id: Iaf4a6f4a464fe97a44f63bf2ca5a1a2e04400b9b
Reviewed-on: https://chromium-review.googlesource.com/958126
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Han Leon <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#542712}
(cherry picked from commit 733a5fa4ef949b7e3357122ffcd58c17eb172c68)

Fix jumbo+Win compilation error related to the PostMessage macro

PostMessage is a macro in Windows and depending on whether Windows
headers have been included or not, PostMessage will either be
treated as PostMessage or PostMessageW. We use a special header
windows_types.h to ensure that the macro is always defined
so that there will be no compilation or linking issues missing
either PostMessage or PostMessageW objects.

Bug:  772713 ,813749

TBR=leon.han@intel.com

(cherry picked from commit c24cb7fb7bc50d9bcdabfe9e82670a418b438039)

Change-Id: I0012774f347ef2e257bc87b4492f58c7be464f78
Reviewed-on: https://chromium-review.googlesource.com/957028
Reviewed-by: Daniel Bratell <bratell@opera.com>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Original-Commit-Position: refs/heads/master@{#542092}
Reviewed-on: https://chromium-review.googlesource.com/974724
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#376}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/content/browser/service_worker/service_worker_handle.cc
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/content/browser/service_worker/service_worker_handle.h
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/content/browser/service_worker/service_worker_handle_unittest.cc
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/content/renderer/service_worker/service_worker_dispatcher_unittest.cc
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/content/renderer/service_worker/service_worker_provider_context_unittest.cc
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/content/renderer/service_worker/web_service_worker_impl.cc
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/content/renderer/service_worker/web_service_worker_impl.h
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/third_party/WebKit/public/mojom/service_worker/service_worker_object.mojom
[modify] https://crrev.com/f2e62a342ee398fe6870ee4a1f3b3a04f1cecb0b/third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorker.h

Sign in to add a comment