A non-Oilpan, lazy sweeping regression

[Sigbjorn Finne]

Hi,

with the r195022 re-land, lazy sweeping is enabled for non-Oilpan
builds. Looking through today's trybot results, I'm wondering if we're
running into an issue,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_ASAN/18437/layout-test-results/http/tests/serviceworker/fetch-canvas-tainting-crash-log.txt

Having non-GCed execution contexts with GCed context lifecycle observers
is something we've deliberated as being safe -- the observer taking care
of de-registering when finalized, so the notifier will have an accurate
& live set of observers to work over.

Which won't hold true now with lazy sweeping, as the observer _and_ the
objects it refers to will not be promptly swept.. risking lifecycle
notifications to indirectly access objects that have been finalized.
Which explains the above stack, ReadableStreamReader hasn't been
finalized, but the ReadableStream object has.

I'm proactively reverting -- if TOK think my analysis is wrong, please
re-land (and sorry :) )

--sigbjorn

[Kentaro Hara]

Thanks Sigbjorn for the investigation!

Which won't hold true now with lazy sweeping, as the observer _and_ the objects it refers to will not be promptly swept.. risking lifecycle notifications to indirectly access objects that have been finalized. Which explains the above stack, ReadableStreamReader hasn't been finalized, but the ReadableStream object has.

The analysis sounds correct. I now remember that we tried hard to move LifecycleObserver's hierarchy to the heap before enabling lazy sweeping on oilpan builds for exactly the same reason...

That being said, it is problematic that we cannot enable lazy sweeping until enabling oilpan for the LifecycleObserver's hierarchy by default.

- We cannot ship oilpan for the LifecycleObserver's hierarchy until we ship oilpan for the Node hierarchy.

- We want to ship oilpan for core/animations/ and html/canvas/ before we ship oilpan for the Node hierarchy.

- Lazy sweeping is mandatory for shipping oilpan for core/animations/ and html/canvas/.

Would it be possible to use the isHeapObjectAlive technique (which we're using in Timer: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/Timer.h&q=timer%20isheapobjectalive&sq=package:chromium&type=cs&l=121) in LifecycleObserver and enable lazy sweeping?

--

Kentaro Hara, Tokyo, Japan

[Sigbjorn Finne]

Den 5/8/2015 02:04, Kentaro Hara skreiv:
>
...

>
> Would it be possible to use the isHeapObjectAlive technique (which we're
> using in Timer:
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/Timer.h&q=timer%20isheapobjectalive&sq=package:chromium&type=cs&l=121)
> in LifecycleObserver and enable lazy sweeping?
>

You'd have to add it to all notifier methods iterating over the observer
set, so a fairly invasive change.

--sibjorn

[Sigbjorn Finne]

Re: footprint of what I had in mind,
https://codereview.chromium.org/1132053002/ (completely untested.)

--sigbjorn

[Kentaro Hara]

Thanks for the CL!

Added a comment to the CL, but I think it would be reasonable to land the CL.

 

[Sigbjørn Finne]

On Sat, May 9, 2015 at 4:51 PM, Kentaro Hara <har...@chromium.org> wrote:
> Thanks for the CL!
>
> Added a comment to the CL, but I think it would be reasonable to land the
> CL.
>

Thanks for reviewing it in detail.

It only handles a subset of these lazy sweeping instabilities though,

- https://code.google.com/p/chromium/issues/detail?id=485913
- https://code.google.com/p/chromium/issues/detail?id=485843

are two others. The latter one is especially problematic, as it puts
into question how we can reliably allow (or prevent) handing out
"client" heap references from Blink to the embedder, for the embedder
to call until some protocol revokes the reference when the client (or
its owner) is destructed.

Upon invocation of each of its methods by the embedder, the client
abstraction will need to check if the object is about to be swept and
abort the call, if it is. That's brittle, but aggressively using
persistents (by way of WebPrivatePtr<>s, perhaps) will lead to leak
problems also. I hope we can find something workable.

--sigbjorn

[Kentaro Hara]

It only handles a subset of these lazy sweeping instabilities though, 

 

 - https://code.google.com/p/chromium/issues/detail?id=485913
 - https://code.google.com/p/chromium/issues/detail?id=485843 

are two others. The latter one is especially problematic, as it puts
into question how we can reliably allow (or prevent) handing out
"client" heap references from Blink to the embedder, for the embedder
to call until some protocol revokes the reference when the client (or
its owner) is destructed. 

 

Upon invocation of each of its methods by the embedder, the client
abstraction will need to check if the object is about to be swept and
abort the call, if it is. That's brittle, but aggressively using
persistents (by way of WebPrivatePtr<>s, perhaps) will lead to leak
problems also. I hope we can find something workable.

Just help me understand: In the latter bug, how is the problem avoided on oilpan bulids? I'm wondering why the problem occurs only on non-oilpan builds.

I'm getting to understand that this is a hard problem. I need some time to think about our next steps.

Just a random idea, but would it be helpful to disable lazy sweeping for selected classes? For example, we can synchronously sweep Timers and ActiveDOMObjects (the sweeping happens just after the marking phase before resuming the mutator).

[Sigbjorn Finne]

Den 5/9/2015 17:44, Kentaro Hara skreiv:
> It only handles a subset of these lazy sweeping instabilities though,
>
> - https://code.google.com/p/chromium/issues/detail?id=485913
> - https://code.google.com/p/chromium/issues/detail?id=485843
>
>
> are two others. The latter one is especially problematic, as it puts
> into question how we can reliably allow (or prevent) handing out
> "client" heap references from Blink to the embedder, for the embedder
> to call until some protocol revokes the reference when the client (or
> its owner) is destructed.
>
> Upon invocation of each of its methods by the embedder, the client
> abstraction will need to check if the object is about to be swept and
> abort the call, if it is. That's brittle, but aggressively using
> persistents (by way of WebPrivatePtr<>s, perhaps) will lead to leak
> problems also. I hope we can find something workable.
>
>
> Just help me understand: In the latter bug, how is the problem avoided
> on oilpan bulids? I'm wondering why the problem occurs only on
> non-oilpan builds.

I think it comes down to not having fuzzers running Oilpan enabled
builds, and other testing beyond layout tests.

>
> I'm getting to understand that this is a hard problem. I need some time
> to think about our next steps.
>
> Just a random idea, but would it be helpful to disable lazy sweeping for
> selected classes? For example, we can synchronously sweep Timers and
> ActiveDOMObjects (the sweeping happens just after the marking phase
> before resuming the mutator).
>

I've been thinking along similar lines -- have certain objects assigned
to 'eagerly swept' heaps, as it would avoid instrumenting
implementations with Heap::willObjectBeLazilySwept() checks. But unless
timers are on the heap, and it is not in other cases too tricky to
identify what objects need to be eagerly swept, I'm not sure it will
make things simpler. But worth thinking about some more.

--sigbjorn

[Kentaro Hara]

I think it comes down to not having fuzzers running Oilpan enabled builds, and other testing beyond layout tests.

Thanks, I'll ask the fuzz team to set up a fuzzer for oilpan enabled builds.

I've been thinking along similar lines -- have certain objects assigned to 'eagerly swept' heaps, as it would avoid instrumenting implementations with Heap::willObjectBeLazilySwept() checks. But unless timers are on the heap, and it is not in other cases too tricky to identify what objects need to be eagerly swept, I'm not sure it will make things simpler. But worth thinking about some more.

Yeah, I'll think about this too.

[Sigbjorn Finne]

Den 5/9/2015 18:51, Kentaro Hara skreiv:
> I think it comes down to not having fuzzers running Oilpan enabled
> builds, and other testing beyond layout tests.
>
>
> Thanks, I'll ask the fuzz team to set up a fuzzer for oilpan enabled builds.
>
>
> I've been thinking along similar lines -- have certain objects
> assigned to 'eagerly swept' heaps, as it would avoid instrumenting
> implementations with Heap::willObjectBeLazilySwept() checks. But
> unless timers are on the heap, and it is not in other cases too
> tricky to identify what objects need to be eagerly swept, I'm not
> sure it will make things simpler. But worth thinking about some more.
>
>
> Yeah, I'll think about this too.
>

https://codereview.chromium.org/1138663002/ adds the facility. I tried
to explain some of the reasoning/motivation for it in the very long
commit message, but not as clear as it could be yet.

--sigbjorn