Bug 70431 – [chromium] Fix teardown in Web*LayerImpl

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Labour]

Fix teardown in Web*LayerImpl

[Antoine Labour]

Created attachment 111648 [details]
Patch

[Antoine Labour]

Since Web*LayerImpl inherit from LayerChromium that may call into its delegate from the destructor, we need to reset the delegate before calling that base destructor.

[James Robinson]

Comment on attachment 111648 [details]
Patch

Would it be possible to make some webkit_unit_tests that exercise creating/destroying WebLayers just to check for crashes like this?

[Antoine Labour]

Created attachment 111713 [details]
Patch

[Antoine Labour]

Added tests. Also fixed a couple of missing setNeedsCommit indicated by the test.

[WebKit Review Bot]

Comment on attachment 111713 [details]
Patch

Attachment 111713 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10180240

[James Robinson]

Comment on attachment 111713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111713&action=review

Yay tests that find bugs! R=me, few things to address while landing.

> Source/WebKit/chromium/tests/WebLayerTest.cpp:16
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

we prefer 2-clause nowadays, see Source/WebCore/LICENSE-APPLE (but put Google Inc on the top line and only include the current year)

> Source/WebKit/chromium/tests/WebLayerTest.cpp:128
> +    EXPECT_EQ(3, textureLayer.textureId());

use "3u" here or the compiler will whine about signed/unsigned mismatch

[Antoine Labour]

Created attachment 111824 [details]
Patch

[Antoine Labour]

(In reply to comment #7)
> (From update of attachment 111713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111713&action=review
> 
> Yay tests that find bugs! R=me, few things to address while landing.
> 
> > Source/WebKit/chromium/tests/WebLayerTest.cpp:16
> > + *     * Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> > + * copyright notice, this list of conditions and the following disclaimer
> > + * in the documentation and/or other materials provided with the
> > + * distribution.
> > + *     * Neither the name of Google Inc. nor the names of its
> > + * contributors may be used to endorse or promote products derived from
> > + * this software without specific prior written permission.
> 
> we prefer 2-clause nowadays, see Source/WebCore/LICENSE-APPLE (but put Google Inc on the top line and only include the current year)

Done.

> 
> > Source/WebKit/chromium/tests/WebLayerTest.cpp:128
> > +    EXPECT_EQ(3, textureLayer.textureId());
> 
> use "3u" here or the compiler will whine about signed/unsigned mismatch

Done.

[Antoine Labour]

Created attachment 112027 [details]
Patch

[Antoine Labour]

Previous patch was breaking LayerChromiumTest (testing the wrong assumption). Fixed now.

[WebKit Review Bot]

Comment on attachment 112027 [details]
Patch

Clearing flags on attachment: 112027

Committed r98393: <http://trac.webkit.org/changeset/98393>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Labour]

Reopenning to reland once http://codereview.chromium.org/8404008 is in (that should fix it).

[James Robinson]

After that lands make sure that Source/WebKit/chromium/DEPS points to a chromium rev including the change so it gets picked up on the build.webkit.org bots.

[Antoine Labour]

Created attachment 112633 [details]
Patch

[Antoine Labour]

New patch with DEPS roll.

[James Robinson]

Comment on attachment 112633 [details]
Patch

R=me

[James Robinson]

Comment on attachment 112633 [details]
Patch

Clearing flags on attachment: 112633

Committed r98630: <http://trac.webkit.org/changeset/98630>

[James Robinson]

All reviewed patches have been landed.  Closing bug.