Null ptr
<rdar://problem/27297153>
Created attachment 297093 [details] patch
Comment on attachment 297093 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297093&action=review > Source/WebCore/loader/FrameLoader.cpp:929 > // because they need to be contained in iframes with the srcdoc. Getting a null frame would mean that we have a top-level frame with an srcdoc document. This is weird unless the iframe that has the srcdoc is not yet (or no longer in the tree), i.e. detached.
Comment on attachment 297093 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297093&action=review >> Source/WebCore/loader/FrameLoader.cpp:929 >> // because they need to be contained in iframes with the srcdoc. > > Getting a null frame would mean that we have a top-level frame with an srcdoc document. This is weird unless the iframe that has the srcdoc is not yet (or no longer in the tree), i.e. detached. Based on the trace, I suspect this would be reproducible if we: 1. Created an iframe with an srcdoc but not add it to the document 2. Add an HTMLImageElement to that srcdoc 3. Set the src attribute on that HTMLImageElement If so, then this comment and assertion are wrong and your fix is indeed right.
> If so, then this comment and assertion are wrong and your fix is indeed > right. The theory has been that we shouldn't even try to do any loads in disconnected frames so we shouldn't get here in the first place. That's why I didn't remove the assert.
(In reply to comment #5) > > If so, then this comment and assertion are wrong and your fix is indeed > > right. > > The theory has been that we shouldn't even try to do any loads in > disconnected frames so we shouldn't get here in the first place. That's why > I didn't remove the assert. I believe that if you set the src of an HTMLImageElement, the load will go through. It goes through even if the HTMLImageElement is itself detached.
I tried this but it doesn't hit the case: <iframe srcdoc="text" onload="test()"></iframe> <script> function test() { const iframe = document.querySelector("iframe"); const contentDocument = iframe.contentDocument; document.body.removeChild(iframe); const img = contentDocument.createElement("img"); img.setAttribute("src", "foo.png"); } </script> Note that this doesn't necessarily have anything to do with srcdoc, local m_frame could be null.
(In reply to comment #7) > I tried this but it doesn't hit the case: > > <iframe srcdoc="text" onload="test()"></iframe> > <script> > function test() { > const iframe = document.querySelector("iframe"); > const contentDocument = iframe.contentDocument; > document.body.removeChild(iframe); > const img = contentDocument.createElement("img"); > img.setAttribute("src", "foo.png"); > } > </script> > > Note that this doesn't necessarily have anything to do with srcdoc, local > m_frame could be null. FrameLoader::m_frame is a reference so if it is null, we have bigger issues.
> FrameLoader::m_frame is a reference so if it is null, we have bigger issues. True, like dead frame loader. Anyway, I don't know how to repro.
Comment on attachment 297093 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297093&action=review Ok, well we tried. I could not write a test case either. > Source/WebCore/loader/FrameLoader.cpp:932 > + if (!frame) nit: I would have used a ternary.
> nit: I would have used a ternary. I like early bailouts for exceptional cases. I feel ternary sort-of signals that both cases are on the same level.
Comment on attachment 297093 [details] patch Clearing flags on attachment: 297093 Committed r209817: <http://trac.webkit.org/changeset/209817>
All reviewed patches have been landed. Closing bug.