Bug 165852 – WebContent crash under WebCore::CachedResource::load in WebCore::FrameLoader::outgoingReferrer const

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

Null ptr

[Antti Koivisto]

<rdar://problem/27297153>

[Antti Koivisto]

Created attachment 297093 [details]
patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Antti Koivisto]

> 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.

[Chris Dumez]

(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.

[Antti Koivisto]

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.

[Chris Dumez]

(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.

[Antti Koivisto]

> 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.

[Chris Dumez]

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.

[Antti Koivisto]

> 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.

[WebKit Commit Bot]

Comment on attachment 297093 [details]
patch

Clearing flags on attachment: 297093

Committed r209817: <http://trac.webkit.org/changeset/209817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.