Remove Oilpan-only StyleSheet/ test failures from TestExpectations

Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328
Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}

Committed: https://crrev.com/01bd400bd56982ab1e1130eda099dec47d7a3242
Cr-Commit-Position: refs/heads/master@{#393824}

[keishi, 2016-04-22 08:10:30]

Removed detached-parent-rule-without-wrapper.html and
detached-stylesheet-without-wrapper.html because what they are testing do not
match the current behavior with the strong reference.

[keishi, 2016-04-22 11:37:45]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[keishi, 2016-04-22 11:37:45]

PTAL

[sof, 2016-04-22 11:41:41]

On 2016/04/22 08:10:30, keishi wrote:
> Removed detached-parent-rule-without-wrapper.html and
> detached-stylesheet-without-wrapper.html because what they are testing do not
> match the current behavior with the strong reference.

Is it problematic to update them to match current behavior?

[keishi, 2016-04-22 11:59:55]

On 2016/04/22 11:41:41, sof wrote:
> On 2016/04/22 08:10:30, keishi wrote:
> > Removed detached-parent-rule-without-wrapper.html and
> > detached-stylesheet-without-wrapper.html because what they are testing do
not
> > match the current behavior with the strong reference.
> 
> Is it problematic to update them to match current behavior?

I removed it because it didn't match the test description but I guess its worth
keeping. Added back.

[sof, 2016-04-22 12:30:56]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-04-22 12:30:57]

https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
(right):

https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
FAIL style.parentRule.foo should be bar (of type string). Was undefined (of type
undefined).
Is this (and the other gc-* tests) expected to fail still?

[keishi, 2016-04-26 05:05:35]

keishi@chromium.org changed reviewers:
+ yukishiino@chromium.org

[keishi, 2016-04-26 05:05:36]

https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
(right):

https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
FAIL style.parentRule.foo should be bar (of type string). Was undefined (of type
undefined).
On 2016/04/22 12:30:57, sof wrote:
> Is this (and the other gc-* tests) expected to fail still?

I was able to make gc-declaration-parent-rule.html pass by adding
SetWrapperReferenceTo to CSSStyleDeclaration.
I tried to do the same for gc-declaration-parent-stylesheet.html but it didn't
work.

The wrapper for parentRule/parentStyleSheet is being disposed during the GC. I'm
not sure why SetWrapperReferenceTo worked.
Can +yukishiino or +haraken teach me how the wrapper lifetime should be managed?

[haraken, 2016-04-26 07:25:49]

On 2016/04/26 05:05:36, keishi wrote:
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> FAIL style.parentRule.foo should be bar (of type string). Was undefined (of
type
> undefined).
> On 2016/04/22 12:30:57, sof wrote:
> > Is this (and the other gc-* tests) expected to fail still?
> 
> I was able to make gc-declaration-parent-rule.html pass by adding
> SetWrapperReferenceTo to CSSStyleDeclaration.
> I tried to do the same for gc-declaration-parent-stylesheet.html but it didn't
> work.
> 
> The wrapper for parentRule/parentStyleSheet is being disposed during the GC.
I'm
> not sure why SetWrapperReferenceTo worked.
> Can +yukishiino or +haraken teach me how the wrapper lifetime should be
managed?

[SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a reference only
when the target wrapper is already created. If the target wrapper does not
exist, [SetWrapperReferenceTo] does nothing.

[SetWrapperReferenceFrom] doesn't have the issue, but can you use
[SetWrapperReferenceFrom] instead?

(This complexity will be gone once the traceWrapper is enabled. Then we can
replace all [SetWrapperReferenceFrom/To] with traceWrappers.)

BTW, I don't fully understand why wrappers need to get involved in
gc-declaration-parent-rule.html. Why isn't it enough to add Oilpan's Members to
keep the parent StyleSheet/StyleRule alive?

[Yuki, 2016-04-26 08:04:25]

On 2016/04/26 07:25:49, haraken wrote:
> On 2016/04/26 05:05:36, keishi wrote:
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > File
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> > (right):
> > 
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> > FAIL style.parentRule.foo should be bar (of type string). Was undefined (of
> type
> > undefined).
> > On 2016/04/22 12:30:57, sof wrote:
> > > Is this (and the other gc-* tests) expected to fail still?
> > 
> > I was able to make gc-declaration-parent-rule.html pass by adding
> > SetWrapperReferenceTo to CSSStyleDeclaration.
> > I tried to do the same for gc-declaration-parent-stylesheet.html but it
didn't
> > work.
> > 
> > The wrapper for parentRule/parentStyleSheet is being disposed during the GC.
> I'm
> > not sure why SetWrapperReferenceTo worked.
> > Can +yukishiino or +haraken teach me how the wrapper lifetime should be
> managed?
> 
> [SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a reference
only
> when the target wrapper is already created. If the target wrapper does not
> exist, [SetWrapperReferenceTo] does nothing.
> 
> [SetWrapperReferenceFrom] doesn't have the issue, but can you use
> [SetWrapperReferenceFrom] instead?
> 
> (This complexity will be gone once the traceWrapper is enabled. Then we can
> replace all [SetWrapperReferenceFrom/To] with traceWrappers.)
> 
> BTW, I don't fully understand why wrappers need to get involved in
> gc-declaration-parent-rule.html. Why isn't it enough to add Oilpan's Members
to
> keep the parent StyleSheet/StyleRule alive?

I heard that V8 doesn't collect an object if a) the object is a DOM wrapper, and
b) the object has an user-defined property.  Is it still true?  Is there any
exception?  I don't see a good reason why the obejct is collected.

[haraken, 2016-04-26 08:05:49]

On 2016/04/26 08:04:25, Yuki wrote:
> On 2016/04/26 07:25:49, haraken wrote:
> > On 2016/04/26 05:05:36, keishi wrote:
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > File
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> > > FAIL style.parentRule.foo should be bar (of type string). Was undefined
(of
> > type
> > > undefined).
> > > On 2016/04/22 12:30:57, sof wrote:
> > > > Is this (and the other gc-* tests) expected to fail still?
> > > 
> > > I was able to make gc-declaration-parent-rule.html pass by adding
> > > SetWrapperReferenceTo to CSSStyleDeclaration.
> > > I tried to do the same for gc-declaration-parent-stylesheet.html but it
> didn't
> > > work.
> > > 
> > > The wrapper for parentRule/parentStyleSheet is being disposed during the
GC.
> > I'm
> > > not sure why SetWrapperReferenceTo worked.
> > > Can +yukishiino or +haraken teach me how the wrapper lifetime should be
> > managed?
> > 
> > [SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a reference
> only
> > when the target wrapper is already created. If the target wrapper does not
> > exist, [SetWrapperReferenceTo] does nothing.
> > 
> > [SetWrapperReferenceFrom] doesn't have the issue, but can you use
> > [SetWrapperReferenceFrom] instead?
> > 
> > (This complexity will be gone once the traceWrapper is enabled. Then we can
> > replace all [SetWrapperReferenceFrom/To] with traceWrappers.)
> > 
> > BTW, I don't fully understand why wrappers need to get involved in
> > gc-declaration-parent-rule.html. Why isn't it enough to add Oilpan's Members
> to
> > keep the parent StyleSheet/StyleRule alive?
> 
> I heard that V8 doesn't collect an object if a) the object is a DOM wrapper,
and
> b) the object has an user-defined property.  Is it still true?  Is there any
> exception?  I don't see a good reason why the obejct is collected.

It is true for minor GCs. It is not true for major GCs. Major GCs collect all
unreachable DOM wrappers.

[Yuki, 2016-04-26 08:11:15]

On 2016/04/26 08:05:49, haraken wrote:
> On 2016/04/26 08:04:25, Yuki wrote:
> > On 2016/04/26 07:25:49, haraken wrote:
> > > On 2016/04/26 05:05:36, keishi wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > File
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> > > > FAIL style.parentRule.foo should be bar (of type string). Was undefined
> (of
> > > type
> > > > undefined).
> > > > On 2016/04/22 12:30:57, sof wrote:
> > > > > Is this (and the other gc-* tests) expected to fail still?
> > > > 
> > > > I was able to make gc-declaration-parent-rule.html pass by adding
> > > > SetWrapperReferenceTo to CSSStyleDeclaration.
> > > > I tried to do the same for gc-declaration-parent-stylesheet.html but it
> > didn't
> > > > work.
> > > > 
> > > > The wrapper for parentRule/parentStyleSheet is being disposed during the
> GC.
> > > I'm
> > > > not sure why SetWrapperReferenceTo worked.
> > > > Can +yukishiino or +haraken teach me how the wrapper lifetime should be
> > > managed?
> > > 
> > > [SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a
reference
> > only
> > > when the target wrapper is already created. If the target wrapper does not
> > > exist, [SetWrapperReferenceTo] does nothing.
> > > 
> > > [SetWrapperReferenceFrom] doesn't have the issue, but can you use
> > > [SetWrapperReferenceFrom] instead?
> > > 
> > > (This complexity will be gone once the traceWrapper is enabled. Then we
can
> > > replace all [SetWrapperReferenceFrom/To] with traceWrappers.)
> > > 
> > > BTW, I don't fully understand why wrappers need to get involved in
> > > gc-declaration-parent-rule.html. Why isn't it enough to add Oilpan's
Members
> > to
> > > keep the parent StyleSheet/StyleRule alive?
> > 
> > I heard that V8 doesn't collect an object if a) the object is a DOM wrapper,
> and
> > b) the object has an user-defined property.  Is it still true?  Is there any
> > exception?  I don't see a good reason why the obejct is collected.
> 
> It is true for minor GCs. It is not true for major GCs. Major GCs collect all
> unreachable DOM wrappers.

Then, are we invoking a major GC?
    |style| is reachable from V8.
    Oilpan makes the C++ DOM obejct for |style.parentRule| alive.
    Who makes the wrapper object for |style.parentRule| alive?

[haraken, 2016-04-26 08:19:55]

On 2016/04/26 08:11:15, Yuki wrote:
> On 2016/04/26 08:05:49, haraken wrote:
> > On 2016/04/26 08:04:25, Yuki wrote:
> > > On 2016/04/26 07:25:49, haraken wrote:
> > > > On 2016/04/26 05:05:36, keishi wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > > File
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> > > > > FAIL style.parentRule.foo should be bar (of type string). Was
undefined
> > (of
> > > > type
> > > > > undefined).
> > > > > On 2016/04/22 12:30:57, sof wrote:
> > > > > > Is this (and the other gc-* tests) expected to fail still?
> > > > > 
> > > > > I was able to make gc-declaration-parent-rule.html pass by adding
> > > > > SetWrapperReferenceTo to CSSStyleDeclaration.
> > > > > I tried to do the same for gc-declaration-parent-stylesheet.html but
it
> > > didn't
> > > > > work.
> > > > > 
> > > > > The wrapper for parentRule/parentStyleSheet is being disposed during
the
> > GC.
> > > > I'm
> > > > > not sure why SetWrapperReferenceTo worked.
> > > > > Can +yukishiino or +haraken teach me how the wrapper lifetime should
be
> > > > managed?
> > > > 
> > > > [SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a
> reference
> > > only
> > > > when the target wrapper is already created. If the target wrapper does
not
> > > > exist, [SetWrapperReferenceTo] does nothing.
> > > > 
> > > > [SetWrapperReferenceFrom] doesn't have the issue, but can you use
> > > > [SetWrapperReferenceFrom] instead?
> > > > 
> > > > (This complexity will be gone once the traceWrapper is enabled. Then we
> can
> > > > replace all [SetWrapperReferenceFrom/To] with traceWrappers.)
> > > > 
> > > > BTW, I don't fully understand why wrappers need to get involved in
> > > > gc-declaration-parent-rule.html. Why isn't it enough to add Oilpan's
> Members
> > > to
> > > > keep the parent StyleSheet/StyleRule alive?
> > > 
> > > I heard that V8 doesn't collect an object if a) the object is a DOM
wrapper,
> > and
> > > b) the object has an user-defined property.  Is it still true?  Is there
any
> > > exception?  I don't see a good reason why the obejct is collected.
> > 
> > It is true for minor GCs. It is not true for major GCs. Major GCs collect
all
> > unreachable DOM wrappers.
> 
> Then, are we invoking a major GC?
>     |style| is reachable from V8.
>     Oilpan makes the C++ DOM obejct for |style.parentRule| alive.
>     Who makes the wrapper object for |style.parentRule| alive?

Ah, I was confused. Now I got it -- the problem is that the wrapper of
style.parentRule is collected, not the DOM object is created. Then I think a
major GC is happening there.

But either way, it's problematic that style.parentRule gets collected in major
GCs. Can we add [SetWrapperReferenceFrom] to the StyleRule.idl (rather than
[SetWrapperReferenceTo])?

[keishi, 2016-04-26 12:31:17]

On 2016/04/26 08:19:55, haraken wrote:
> On 2016/04/26 08:11:15, Yuki wrote:
> > On 2016/04/26 08:05:49, haraken wrote:
> > > On 2016/04/26 08:04:25, Yuki wrote:
> > > > On 2016/04/26 07:25:49, haraken wrote:
> > > > > On 2016/04/26 05:05:36, keishi wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > > > File
> > > > > >
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> > > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > > >
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> > > > > > FAIL style.parentRule.foo should be bar (of type string). Was
> undefined
> > > (of
> > > > > type
> > > > > > undefined).
> > > > > > On 2016/04/22 12:30:57, sof wrote:
> > > > > > > Is this (and the other gc-* tests) expected to fail still?
> > > > > > 
> > > > > > I was able to make gc-declaration-parent-rule.html pass by adding
> > > > > > SetWrapperReferenceTo to CSSStyleDeclaration.
> > > > > > I tried to do the same for gc-declaration-parent-stylesheet.html but
> it
> > > > didn't
> > > > > > work.
> > > > > > 
> > > > > > The wrapper for parentRule/parentStyleSheet is being disposed during
> the
> > > GC.
> > > > > I'm
> > > > > > not sure why SetWrapperReferenceTo worked.
> > > > > > Can +yukishiino or +haraken teach me how the wrapper lifetime should
> be
> > > > > managed?
> > > > > 
> > > > > [SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a
> > reference
> > > > only
> > > > > when the target wrapper is already created. If the target wrapper does
> not
> > > > > exist, [SetWrapperReferenceTo] does nothing.
> > > > > 
> > > > > [SetWrapperReferenceFrom] doesn't have the issue, but can you use
> > > > > [SetWrapperReferenceFrom] instead?
> > > > > 
> > > > > (This complexity will be gone once the traceWrapper is enabled. Then
we
> > can
> > > > > replace all [SetWrapperReferenceFrom/To] with traceWrappers.)
> > > > > 
> > > > > BTW, I don't fully understand why wrappers need to get involved in
> > > > > gc-declaration-parent-rule.html. Why isn't it enough to add Oilpan's
> > Members
> > > > to
> > > > > keep the parent StyleSheet/StyleRule alive?
> > > > 
> > > > I heard that V8 doesn't collect an object if a) the object is a DOM
> wrapper,
> > > and
> > > > b) the object has an user-defined property.  Is it still true?  Is there
> any
> > > > exception?  I don't see a good reason why the obejct is collected.
> > > 
> > > It is true for minor GCs. It is not true for major GCs. Major GCs collect
> all
> > > unreachable DOM wrappers.
> > 
> > Then, are we invoking a major GC?
> >     |style| is reachable from V8.
> >     Oilpan makes the C++ DOM obejct for |style.parentRule| alive.
> >     Who makes the wrapper object for |style.parentRule| alive?
> 
> Ah, I was confused. Now I got it -- the problem is that the wrapper of
> style.parentRule is collected, not the DOM object is created. Then I think a
> major GC is happening there.
> 
> But either way, it's problematic that style.parentRule gets collected in major
> GCs. Can we add [SetWrapperReferenceFrom] to the StyleRule.idl (rather than
> [SetWrapperReferenceTo])?

SetWrapperReferenceFrom only supports Node.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I modified the binding generator to support different types but it still didn't
work.
My guess is that SetReferenceFromGroup only supports Node?

[haraken, 2016-04-26 12:36:32]

On 2016/04/26 12:31:17, keishi wrote:
> On 2016/04/26 08:19:55, haraken wrote:
> > On 2016/04/26 08:11:15, Yuki wrote:
> > > On 2016/04/26 08:05:49, haraken wrote:
> > > > On 2016/04/26 08:04:25, Yuki wrote:
> > > > > On 2016/04/26 07:25:49, haraken wrote:
> > > > > > On 2016/04/26 05:05:36, keishi wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > > > > File
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> > > > > > > (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> > > > > > > FAIL style.parentRule.foo should be bar (of type string). Was
> > undefined
> > > > (of
> > > > > > type
> > > > > > > undefined).
> > > > > > > On 2016/04/22 12:30:57, sof wrote:
> > > > > > > > Is this (and the other gc-* tests) expected to fail still?
> > > > > > > 
> > > > > > > I was able to make gc-declaration-parent-rule.html pass by adding
> > > > > > > SetWrapperReferenceTo to CSSStyleDeclaration.
> > > > > > > I tried to do the same for gc-declaration-parent-stylesheet.html
but
> > it
> > > > > didn't
> > > > > > > work.
> > > > > > > 
> > > > > > > The wrapper for parentRule/parentStyleSheet is being disposed
during
> > the
> > > > GC.
> > > > > > I'm
> > > > > > > not sure why SetWrapperReferenceTo worked.
> > > > > > > Can +yukishiino or +haraken teach me how the wrapper lifetime
should
> > be
> > > > > > managed?
> > > > > > 
> > > > > > [SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a
> > > reference
> > > > > only
> > > > > > when the target wrapper is already created. If the target wrapper
does
> > not
> > > > > > exist, [SetWrapperReferenceTo] does nothing.
> > > > > > 
> > > > > > [SetWrapperReferenceFrom] doesn't have the issue, but can you use
> > > > > > [SetWrapperReferenceFrom] instead?
> > > > > > 
> > > > > > (This complexity will be gone once the traceWrapper is enabled. Then
> we
> > > can
> > > > > > replace all [SetWrapperReferenceFrom/To] with traceWrappers.)
> > > > > > 
> > > > > > BTW, I don't fully understand why wrappers need to get involved in
> > > > > > gc-declaration-parent-rule.html. Why isn't it enough to add Oilpan's
> > > Members
> > > > > to
> > > > > > keep the parent StyleSheet/StyleRule alive?
> > > > > 
> > > > > I heard that V8 doesn't collect an object if a) the object is a DOM
> > wrapper,
> > > > and
> > > > > b) the object has an user-defined property.  Is it still true?  Is
there
> > any
> > > > > exception?  I don't see a good reason why the obejct is collected.
> > > > 
> > > > It is true for minor GCs. It is not true for major GCs. Major GCs
collect
> > all
> > > > unreachable DOM wrappers.
> > > 
> > > Then, are we invoking a major GC?
> > >     |style| is reachable from V8.
> > >     Oilpan makes the C++ DOM obejct for |style.parentRule| alive.
> > >     Who makes the wrapper object for |style.parentRule| alive?
> > 
> > Ah, I was confused. Now I got it -- the problem is that the wrapper of
> > style.parentRule is collected, not the DOM object is created. Then I think a
> > major GC is happening there.
> > 
> > But either way, it's problematic that style.parentRule gets collected in
major
> > GCs. Can we add [SetWrapperReferenceFrom] to the StyleRule.idl (rather than
> > [SetWrapperReferenceTo])?
> 
> SetWrapperReferenceFrom only supports Node.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I modified the binding generator to support different types but it still
didn't
> work.
> My guess is that SetReferenceFromGroup only supports Node?

Yeah... you're right. Object grouping is supported only for Nodes.

Would [Custom=VisitDOMWrapper] work? With [Custom=VisitDOMWrapper], you can
define any wrapper reachability in the custom bindings.

[keishi, 2016-04-27 12:36:13]

On 2016/04/26 12:36:32, haraken wrote:
> On 2016/04/26 12:31:17, keishi wrote:
> > On 2016/04/26 08:19:55, haraken wrote:
> > > On 2016/04/26 08:11:15, Yuki wrote:
> > > > On 2016/04/26 08:05:49, haraken wrote:
> > > > > On 2016/04/26 08:04:25, Yuki wrote:
> > > > > > On 2016/04/26 07:25:49, haraken wrote:
> > > > > > > On 2016/04/26 05:05:36, keishi wrote:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > > > > > File
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt
> > > > > > > > (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1904423002/diff/40001/third_party/WebKit/Layo...
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-declaration-parent-rule-expected.txt:6:
> > > > > > > > FAIL style.parentRule.foo should be bar (of type string). Was
> > > undefined
> > > > > (of
> > > > > > > type
> > > > > > > > undefined).
> > > > > > > > On 2016/04/22 12:30:57, sof wrote:
> > > > > > > > > Is this (and the other gc-* tests) expected to fail still?
> > > > > > > > 
> > > > > > > > I was able to make gc-declaration-parent-rule.html pass by
adding
> > > > > > > > SetWrapperReferenceTo to CSSStyleDeclaration.
> > > > > > > > I tried to do the same for gc-declaration-parent-stylesheet.html
> but
> > > it
> > > > > > didn't
> > > > > > > > work.
> > > > > > > > 
> > > > > > > > The wrapper for parentRule/parentStyleSheet is being disposed
> during
> > > the
> > > > > GC.
> > > > > > > I'm
> > > > > > > > not sure why SetWrapperReferenceTo worked.
> > > > > > > > Can +yukishiino or +haraken teach me how the wrapper lifetime
> should
> > > be
> > > > > > > managed?
> > > > > > > 
> > > > > > > [SetWrapperReferenceTo] is tricky. [SetWrapperReferenceTo] adds a
> > > > reference
> > > > > > only
> > > > > > > when the target wrapper is already created. If the target wrapper
> does
> > > not
> > > > > > > exist, [SetWrapperReferenceTo] does nothing.
> > > > > > > 
> > > > > > > [SetWrapperReferenceFrom] doesn't have the issue, but can you use
> > > > > > > [SetWrapperReferenceFrom] instead?
> > > > > > > 
> > > > > > > (This complexity will be gone once the traceWrapper is enabled.
Then
> > we
> > > > can
> > > > > > > replace all [SetWrapperReferenceFrom/To] with traceWrappers.)
> > > > > > > 
> > > > > > > BTW, I don't fully understand why wrappers need to get involved in
> > > > > > > gc-declaration-parent-rule.html. Why isn't it enough to add
Oilpan's
> > > > Members
> > > > > > to
> > > > > > > keep the parent StyleSheet/StyleRule alive?
> > > > > > 
> > > > > > I heard that V8 doesn't collect an object if a) the object is a DOM
> > > wrapper,
> > > > > and
> > > > > > b) the object has an user-defined property.  Is it still true?  Is
> there
> > > any
> > > > > > exception?  I don't see a good reason why the obejct is collected.
> > > > > 
> > > > > It is true for minor GCs. It is not true for major GCs. Major GCs
> collect
> > > all
> > > > > unreachable DOM wrappers.
> > > > 
> > > > Then, are we invoking a major GC?
> > > >     |style| is reachable from V8.
> > > >     Oilpan makes the C++ DOM obejct for |style.parentRule| alive.
> > > >     Who makes the wrapper object for |style.parentRule| alive?
> > > 
> > > Ah, I was confused. Now I got it -- the problem is that the wrapper of
> > > style.parentRule is collected, not the DOM object is created. Then I think
a
> > > major GC is happening there.
> > > 
> > > But either way, it's problematic that style.parentRule gets collected in
> major
> > > GCs. Can we add [SetWrapperReferenceFrom] to the StyleRule.idl (rather
than
> > > [SetWrapperReferenceTo])?
> > 
> > SetWrapperReferenceFrom only supports Node.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > I modified the binding generator to support different types but it still
> didn't
> > work.
> > My guess is that SetReferenceFromGroup only supports Node?
> 
> Yeah... you're right. Object grouping is supported only for Nodes.
> 
> Would [Custom=VisitDOMWrapper] work? With [Custom=VisitDOMWrapper], you can
> define any wrapper reachability in the custom bindings.

Now its passing all the tests.
Looks like bindings only handles one SetWrapperReferenceTo so creating a custom
VisitDOMWrapper that adds reference to parentRule and parentStyleSheet solved
the issue.

Seems like I should be adding VisitDOMWrapper for all CSSRule subclasses but
thats a lot of custom bindings so I am just adding it to CSSStyleRule in this
CL.

[haraken, 2016-04-27 15:57:34]

LGTM. Thanks for the digging!

https://codereview.chromium.org/1904423002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSStyleRule.idl (right):

https://codereview.chromium.org/1904423002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSStyleRule.idl:24: DependentLifetime,

I guess we won't need [DependentLifetime] since [Custom=VisitDOMWrapper] implies
it.

[keishi, 2016-04-28 01:56:09]

https://codereview.chromium.org/1904423002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSStyleRule.idl (right):

https://codereview.chromium.org/1904423002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSStyleRule.idl:24: DependentLifetime,
On 2016/04/27 15:57:34, haraken wrote:
> 
> I guess we won't need [DependentLifetime] since [Custom=VisitDOMWrapper]
implies
> it.
> 

Done.

[keishi, 2016-04-28 01:56:30]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-04-28 01:56:30]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1904423002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-04-28 01:56:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904423002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904423002/100001

[commit-bot: I haz the power, 2016-04-28 03:16:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:15:49]

Patchset 6 (id:??) landed as
https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}

[commit-bot: I haz the power, 2016-04-30 17:53:56]

Description was changed from

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328
==========

to

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328

Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}
==========

[jochen (gone - plz use gerrit), 2016-05-11 08:43:52]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1968903002/ by jochen@chromium.org.

The reason for reverting is: speculative revert to see whether this caused
crbug.com/610340

BUG=610340.

[keishi, 2016-05-16 11:03:59]

Description was changed from

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328

Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}
==========

to

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328

Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}
==========

[keishi, 2016-05-16 11:22:58]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-05-16 11:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1904423002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-05-16 11:23:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904423002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904423002/120001

[commit-bot: I haz the power, 2016-05-16 12:56:28]

Description was changed from

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328

Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}
==========

to

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328

Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}
==========

[commit-bot: I haz the power, 2016-05-16 12:56:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-16 12:58:16]

Description was changed from

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328

Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}
==========

to

==========
Remove Oilpan-only StyleSheet/ test failures from TestExpectations

BUG=585328

Committed: https://crrev.com/23d02793c61cf69b1818cd007cc39db1d2238b09
Cr-Commit-Position: refs/heads/master@{#390285}

Committed: https://crrev.com/01bd400bd56982ab1e1130eda099dec47d7a3242
Cr-Commit-Position: refs/heads/master@{#393824}
==========

[commit-bot: I haz the power, 2016-05-16 12:58:18]

Patchset 7 (id:??) landed as
https://crrev.com/01bd400bd56982ab1e1130eda099dec47d7a3242
Cr-Commit-Position: refs/heads/master@{#393824}