Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2752203005: Correctly track cross-thread pending FetchEvents. (Closed)

Created:
8 months, 1 week ago by sof
Modified:
8 months ago
CC:
chromium-reviews, blink-reviews, kinuko+watch
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Correctly track cross-thread pending FetchEvents. ServiceWorkerGlobalScopeProxy is an object that resides on the main thread heap, but is passed to the embedder and called on the (service) worker thread. As the object is GC managed by the main thread, the worker thread cannot update the proxy object with references to heap objects residing in its heap, as the per-thread heap design assumes that each heap only keeps local heap references (via Member<T> and similar.) References across heaps must instead be kept and handled by explicit CrossThreadPersistent<> references. This per-thread heap rule was not being followed for the tracking of pending FetchEvents; adjust the map representation to do so. R=haraken BUG=702527 Review-Url: https://codereview.chromium.org/2752203005 Cr-Commit-Position: refs/heads/master@{#457970} Committed: https://chromium.googlesource.com/chromium/src/+/d0ff3d34a5798ddc3db10815d49dc1687e4f866e

Patch Set 1 #

Patch Set 2 : add comment #

Messages

Total messages: 21 (15 generated)
sof
please take a look.
8 months, 1 week ago (2017-03-18 07:26:03 UTC) #10
haraken
LGTM Why didn't we hit the cross-thread-heap-pointer dcheck during the marking phase? Just by accident?
8 months, 1 week ago (2017-03-18 14:42:43 UTC) #13
sof
On 2017/03/18 14:42:43, haraken wrote: > LGTM > > Why didn't we hit the cross-thread-heap-pointer ...
8 months, 1 week ago (2017-03-18 15:27:54 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2752203005/20001
8 months, 1 week ago (2017-03-18 15:47:25 UTC) #17
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d0ff3d34a5798ddc3db10815d49dc1687e4f866e
8 months, 1 week ago (2017-03-18 15:52:37 UTC) #20
falken
8 months ago (2017-03-23 05:42:10 UTC) #21
Message was sent while issue was closed.
On 2017/03/18 15:27:54, sof wrote:
> On 2017/03/18 14:42:43, haraken wrote:
> > LGTM
> > 
> > Why didn't we hit the cross-thread-heap-pointer dcheck during the marking
> phase?
> > Just by accident?
> 
> I believe so (but don't know), tests do not trigger GCs afterwards for this to
> have been detected.
> 
> I can't see many layout tests for the feature, but would be interested to know
> more about test status
> (http/tests/serviceworker/navigation-preload/chromium/use-counter.html seems
> relevant, but what else?)

Sorry for lack of response here. Most navigation preload tests are in
external/wpt/service-workers/service-worker/navigation-preload.

Powered by Google App Engine
This is Rietveld efc10ee0f