Document calls related to the Storage Access API can be called when the Document is not attached to a frame. Rather than generating a nullptr crash, we should handle this gracefully.
<rdar://problem/40028265>
Created attachment 342102 [details] Patch
Comment on attachment 342102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342102&action=review > LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:8 > +TEST COMPLETE Is it possible to write the logic for this test such that this is the last line that is printed? If so, I suggest we do this for readability and consistency with the output of other tests. > LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:9 > +Test that querying storage access API called on a detached frame doesn't crash. This repeats the description at the top. > LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:11 > +[object HTMLDocument] How does a person interpret this output? Is it necessary to emit this? Can we rewrite the test to emit a PASS/FAIL style message? > LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:4 > + <script src="/js-test-resources/js-test.js"></script> It seems excessive to use js-test for a test that just checks we don’t crash as we are significantly underutilizing its functionality. It seems sufficient to have a description in markup (like you have), have a PASS message in markup and call window.testRunner..dumpAsText(), window.testRunner.waitUntilDone() and window.testRunner.notifyDone() directly. Obviously if the test crashes then there will not be a PASS message. > LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:10 > + var o2 = document.getElementById('test'); Extra space characters after o2. Is it necessary to embed the iframe in the <div>? Would the test work if we inserted the iframe as a child of document.body? Can we come up with a more descriptive name for this variable? The quoting style in this line is inconsistent with the quoting style used throughout this file. This is the only line in this file that uses a single quoted string literal. I suggest that we use a double-quoted string literal for consistency. Regardless we should pick one quoting style and stick with it throughout this document. > LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:12 > + o2.appendChild(testFrame); Is it necessary to dynamically create an iframe? I mean, could this test have been written with a declarative iframe? > LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:14 > + testFrame.outerHTML = testFrameDocument; Is this necessary? > LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:23 > + <p>Test that querying storage access API called on a detached frame doesn't crash.</p> Can we please remove this as it repetitive with the description() text? > LayoutTests/http/tests/storageAccess/request-storage-access-crash-expected.txt:8 > +TEST COMPLETE All of the comments made in has-storage-access-crash-expected.txt apply to this file as well. > LayoutTests/http/tests/storageAccess/request-storage-access-crash.html:10 > + var o2 = document.getElementById('test'); All of the comments made in has-storage-access-crash-expected.html aply to this file as well.
Comment on attachment 342102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342102&action=review >> LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:9 >> +Test that querying storage access API called on a detached frame doesn't crash. > > This repeats the description at the top. Will fix. >> LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:11 >> +[object HTMLDocument] > > How does a person interpret this output? Is it necessary to emit this? Can we rewrite the test to emit a PASS/FAIL style message? This output gets generated when the test frame's outerHTML property is set to test frame's contentDocument. I don't know of a good way to turn this into a PASS/FAIL and still get the bad behavior this test is trying to trigger in the DOM. >> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:4 >> + <script src="/js-test-resources/js-test.js"></script> > > It seems excessive to use js-test for a test that just checks we don’t crash as we are significantly underutilizing its functionality. It seems sufficient to have a description in markup (like you have), have a PASS message in markup and call window.testRunner..dumpAsText(), window.testRunner.waitUntilDone() and window.testRunner.notifyDone() directly. Obviously if the test crashes then there will not be a PASS message. Sounds good. I'll change the test. >> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:10 >> + var o2 = document.getElementById('test'); > > Extra space characters after o2. Is it necessary to embed the iframe in the <div>? Would the test work if we inserted the iframe as a child of document.body? Can we come up with a more descriptive name for this variable? The quoting style in this line is inconsistent with the quoting style used throughout this file. This is the only line in this file that uses a single quoted string literal. I suggest that we use a double-quoted string literal for consistency. Regardless we should pick one quoting style and stick with it throughout this document. Good point! Fixed. >> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:12 >> + o2.appendChild(testFrame); > > Is it necessary to dynamically create an iframe? I mean, could this test have been written with a declarative iframe? Yes. >> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:14 >> + testFrame.outerHTML = testFrameDocument; > > Is this necessary? Yes. >> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:23 >> + <p>Test that querying storage access API called on a detached frame doesn't crash.</p> > > Can we please remove this as it repetitive with the description() text? Done.
Created attachment 342172 [details] Patch for landing
Comment on attachment 342172 [details] Patch for landing Clearing flags on attachment: 342172 Committed r232584: <https://trac.webkit.org/changeset/232584>
All reviewed patches have been landed. Closing bug.