Bug 35796

Summary:

Accept and bailout NULL widgets in ChromiumBridge

Product:

WebKit

Reporter:

anton muhin <antonm>

Component:

WebKit Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fishd

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	fishd: review+, fishd: commit-queue+

anton muhin

Reported 2010-03-05 10:05:29 PST

Accept and bailout NULL widgets in ChromiumBridge

Attachments
Patch (1.20 KB, patch) 2010-03-05 10:09 PST, anton muhin	no flags	Details Formatted Diff Diff
Patch (3.21 KB, patch) 2010-03-09 07:34 PST, anton muhin	no flags	Details Formatted Diff Diff
Patch (3.21 KB, patch) 2010-03-09 07:47 PST, anton muhin	no flags	Details Formatted Diff Diff
Patch (3.21 KB, patch) 2010-03-09 08:59 PST, anton muhin	fishd: review+ fishd: commit-queue+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

anton muhin

Comment 1 2010-03-05 10:09:15 PST

Created attachment 50102 [details] Patch

anton muhin

Comment 2 2010-03-05 10:10:42 PST

This a fix for http://code.google.com/p/chromium/issues/detail?id=36945

Darin Fisher (:fishd, Google)

Comment 3 2010-03-05 11:32:23 PST

Comment on attachment 50102 [details] Patch > diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog > index 93a474b..f2de64e 100644 > --- a/WebKit/chromium/ChangeLog > +++ b/WebKit/chromium/ChangeLog > @@ -1,3 +1,22 @@ > +2010-03-05 Anton Muhin <antonm@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Accept and bailout NULL widgets in ChromiumBridge > + https://bugs.webkit.org/show_bug.cgi?id=35796 > + > + * src/ChromiumBridge.cpp: > + (WebCore::toChromeClientImpl): > + > +2010-03-05 anton muhin <antonm@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Need a short description and bug URL (OOPS!) > + > + * src/ChromiumBridge.cpp: > + (WebCore::toChromeClientImpl): ^^^ duplicate changelog entries? also, please create a layout test for this.

anton muhin

Comment 4 2010-03-09 07:34:16 PST

Created attachment 50304 [details] Patch

anton muhin

Comment 5 2010-03-09 07:37:47 PST

(In reply to comment #3) > (From update of attachment 50102 [details]) > > diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog > > index 93a474b..f2de64e 100644 > > --- a/WebKit/chromium/ChangeLog > > +++ b/WebKit/chromium/ChangeLog > > @@ -1,3 +1,22 @@ > > +2010-03-05 Anton Muhin <antonm@chromium.org> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + Accept and bailout NULL widgets in ChromiumBridge > > + https://bugs.webkit.org/show_bug.cgi?id=35796 > > + > > + * src/ChromiumBridge.cpp: > > + (WebCore::toChromeClientImpl): > > + > > +2010-03-05 anton muhin <antonm@google.com> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + Need a short description and bug URL (OOPS!) > > + > > + * src/ChromiumBridge.cpp: > > + (WebCore::toChromeClientImpl): > > ^^^ duplicate changelog entries? > > also, please create a layout test for this. Darin, sorry for duplicate entry---removed. And layout test added. May you have another look?

anton muhin

Comment 6 2010-03-09 07:47:34 PST

Created attachment 50307 [details] Patch

Darin Fisher (:fishd, Google)

Comment 7 2010-03-09 08:52:55 PST

Comment on attachment 50307 [details] Patch > +++ b/LayoutTests/fast/frames/iframe-access-screen-of-deleted.html > @@ -0,0 +1,36 @@ > +<html> > +<head> > + <script> > + function accessAttributes(s) { > + var value = 0; > + value = s.height; > + value = s.width; > + value = s.colorDepth; > + value = s.pixelDepth; > + value = s.availLeft; > + value = s.availTop; > + value = s.availHeight; > + value = s.availWidth; > + } > + > + function runTests() { > + if (window.layoutTestController) > + layoutTestController.dumpAsText(); > + > + var f = document.getElementById('theframe'); > + var s = f.contentWindow.screen; > + accessAttributes(s); > + > + // Now remove and check that we don't crash. > + f.parentNode.removeChild(f); > + accessAttributes(s); > + } > + </script> > +</head> > +<iframe id="theframe" src="resources/red.html"></iframe> > +<body onload="runTests()"> > +<div> > +This tests that accessing screen attributes doesn't crash even if containing form is removed from the parent. nit: "form" -> "frame"

anton muhin

Comment 8 2010-03-09 08:59:44 PST

Created attachment 50312 [details] Patch

anton muhin

Comment 9 2010-03-09 09:00:57 PST

(In reply to comment #7) > (From update of attachment 50307 [details]) > > +++ b/LayoutTests/fast/frames/iframe-access-screen-of-deleted.html > > @@ -0,0 +1,36 @@ > > +<html> > > +<head> > > + <script> > > + function accessAttributes(s) { > > + var value = 0; > > + value = s.height; > > + value = s.width; > > + value = s.colorDepth; > > + value = s.pixelDepth; > > + value = s.availLeft; > > + value = s.availTop; > > + value = s.availHeight; > > + value = s.availWidth; > > + } > > + > > + function runTests() { > > + if (window.layoutTestController) > > + layoutTestController.dumpAsText(); > > + > > + var f = document.getElementById('theframe'); > > + var s = f.contentWindow.screen; > > + accessAttributes(s); > > + > > + // Now remove and check that we don't crash. > > + f.parentNode.removeChild(f); > > + accessAttributes(s); > > + } > > + </script> > > +</head> > > +<iframe id="theframe" src="resources/red.html"></iframe> > > +<body onload="runTests()"> > > +<div> > > +This tests that accessing screen attributes doesn't crash even if containing form is removed from the parent. > > nit: "form" -> "frame" Fixed. Thanks a lot, Darin. Could you r+ the last patch as well?

anton muhin

Comment 10 2010-03-10 04:30:14 PST

Landed by Darin: http://trac.webkit.org/changeset/55748

Note You need to log in before you can comment on or make changes to this bug.