Bug 70431 - [chromium] Fix teardown in Web*LayerImpl
Summary: [chromium] Fix teardown in Web*LayerImpl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 70892
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-19 11:02 PDT by Antoine Labour
Modified: 2011-10-27 13:41 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.52 KB, patch)
2011-10-19 11:02 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (11.35 KB, patch)
2011-10-19 19:59 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2011-10-20 12:19 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (13.42 KB, patch)
2011-10-21 15:03 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (13.86 KB, patch)
2011-10-26 19:31 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Labour 2011-10-19 11:02:27 PDT
Fix teardown in Web*LayerImpl
Comment 1 Antoine Labour 2011-10-19 11:02:58 PDT
Created attachment 111648 [details]
Patch
Comment 2 Antoine Labour 2011-10-19 11:05:03 PDT
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.
Comment 3 James Robinson 2011-10-19 11:25:00 PDT
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?
Comment 4 Antoine Labour 2011-10-19 19:59:59 PDT
Created attachment 111713 [details]
Patch
Comment 5 Antoine Labour 2011-10-19 20:00:42 PDT
Added tests. Also fixed a couple of missing setNeedsCommit indicated by the test.
Comment 6 WebKit Review Bot 2011-10-19 20:17:58 PDT
Comment on attachment 111713 [details]
Patch

Attachment 111713 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10180240
Comment 7 James Robinson 2011-10-19 20:22:20 PDT
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
Comment 8 Antoine Labour 2011-10-20 12:19:05 PDT
Created attachment 111824 [details]
Patch
Comment 9 Antoine Labour 2011-10-20 12:19:29 PDT
(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.
Comment 10 Antoine Labour 2011-10-21 15:03:32 PDT
Created attachment 112027 [details]
Patch
Comment 11 Antoine Labour 2011-10-21 15:05:04 PDT
Previous patch was breaking LayerChromiumTest (testing the wrong assumption). Fixed now.
Comment 12 WebKit Review Bot 2011-10-25 14:09:19 PDT
Comment on attachment 112027 [details]
Patch

Clearing flags on attachment: 112027

Committed r98393: <http://trac.webkit.org/changeset/98393>
Comment 13 WebKit Review Bot 2011-10-25 14:09:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Antoine Labour 2011-10-26 14:46:30 PDT
Reopenning to reland once http://codereview.chromium.org/8404008 is in (that should fix it).
Comment 15 James Robinson 2011-10-26 16:35:57 PDT
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.
Comment 16 Antoine Labour 2011-10-26 19:31:31 PDT
Created attachment 112633 [details]
Patch
Comment 17 Antoine Labour 2011-10-26 19:31:55 PDT
New patch with DEPS roll.
Comment 18 James Robinson 2011-10-27 13:38:16 PDT
Comment on attachment 112633 [details]
Patch

R=me
Comment 19 James Robinson 2011-10-27 13:41:13 PDT
Comment on attachment 112633 [details]
Patch

Clearing flags on attachment: 112633

Committed r98630: <http://trac.webkit.org/changeset/98630>
Comment 20 James Robinson 2011-10-27 13:41:17 PDT
All reviewed patches have been landed.  Closing bug.