RESOLVED FIXED 35796
Accept and bailout NULL widgets in ChromiumBridge 
https://bugs.webkit.org/show_bug.cgi?id=35796
Summary Accept and bailout NULL widgets in ChromiumBridge
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
DetailsFormatted DiffDiff
Patch (3.21 KB, patch)
2010-03-09 07:34 PST, anton muhin 		no flags
DetailsFormatted DiffDiff
Patch (3.21 KB, patch)
2010-03-09 07:47 PST, anton muhin 		no flags
DetailsFormatted DiffDiff
Patch (3.21 KB, patch)
2010-03-09 08:59 PST, anton muhin 		fishd: review+
fishd: commit-queue+
DetailsFormatted DiffDiff
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.