Bug 49072 – Loader cleanup : Simplify FrameLoader/DocumentLoader setupForReplace()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

Cut down on the back-and-forth function calls between FrameLoader and DocumentLoader when configuring them for multipart content. Move a few lines from DocumentLoader to FrameLoader (the ones that were calling stuff on FrameLoader anyway).

[Nate Chapin]

Created attachment 73074 [details]
patch

[Adam Barth]

Comment on attachment 73074 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73074&action=review

> WebCore/loader/FrameLoader.cpp:1224
> +    writer()->end();

writer() should eventually move to DocumentLoader (because it's scoped to a document rather than to a frame).  I'm not sure how that impacts your thinking about this patch.

[Nate Chapin]

Created attachment 73080 [details]
Move DocumentWriter call back to DocumentLoader

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 73074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73074&action=review
> 
> > WebCore/loader/FrameLoader.cpp:1224
> > +    writer()->end();
> 
> writer() should eventually move to DocumentLoader (because it's scoped to a document rather than to a frame).  I'm not sure how that impacts your thinking about this patch.

It changes one line :)

[Adam Barth]

Comment on attachment 73080 [details]
Move DocumentWriter call back to DocumentLoader

Wow, the old code is so confusing I'm having trouble tracking it even in this patch.  In any case, the minus lines seem to have it.  Thanks.  (Can I convince you to move DocumentWriter to DocumentLoader next?)

[Darin Adler]

I like the idea of this patch, and looked it over earlier today. But I wanted to verify this does not change behavior, which requires understanding thinks like the fact that m_response on the active document loader will always be the same as the response in the local variable named r in MainResourceLoader::didReceiveResponse.

[Nate Chapin]

(In reply to comment #6)
> I like the idea of this patch, and looked it over earlier today. But I wanted to verify this does not change behavior, which requires understanding thinks like the fact that m_response on the active document loader will always be the same as the response in the local variable named r in MainResourceLoader::didReceiveResponse.

The only behavior that I know that I changed is that this patch causes us to always revert the DocumentLoader back to provisional state.  Previously, we were not doing so if the old content was loaded progressive and the new content wasn't.  So I believe the only thing that's changing is whether the active document loader is the provisional or committed loader.

I can hold of on cq if you'd like me to look into this more.

[Darin Adler]

(In reply to comment #7)
> The only behavior that I know that I changed is that this patch causes us to always revert the DocumentLoader back to provisional state.

What effect does that have?

>Previously, we were not doing so if the old content was loaded progressive and the new content wasn't.  So I believe the only thing that's changing is whether the active document loader is the provisional or committed loader.

I don’t understand that sentence well enough. The provisional state is used before we commit. It would be a problem if we got stuck in the provisional state.

> I can hold of on cq if you'd like me to look into this more.

I don’t have any specific requests, but I do want to understand how you proved to yourself this was safe, and make sure you did some testing.

[Nate Chapin]

(In reply to comment #8)
> (In reply to comment #7)
> > The only behavior that I know that I changed is that this patch causes us to always revert the DocumentLoader back to provisional state.
> 
> What effect does that have?
> 
> >Previously, we were not doing so if the old content was loaded progressive and the new content wasn't.  So I believe the only thing that's changing is whether the active document loader is the provisional or committed loader.
> 
> I don’t understand that sentence well enough. The provisional state is used before we commit. It would be a problem if we got stuck in the provisional state.
> 
> > I can hold of on cq if you'd like me to look into this more.
> 
> I don’t have any specific requests, but I do want to understand how you proved to yourself this was safe, and make sure you did some testing.

I went ahead and cleared cq for now.  I confirmed all the layout tests still pass, but I didn't do much testing beyond that (suggestions welcome).

I'll post some more detail about what I think I'm changing a little later.

[Nate Chapin]

(In reply to comment #8)
> (In reply to comment #7)
> > The only behavior that I know that I changed is that this patch causes us to always revert the DocumentLoader back to provisional state.
> 
> What effect does that have?
> 
> >Previously, we were not doing so if the old content was loaded progressive and the new content wasn't.  So I believe the only thing that's changing is whether the active document loader is the provisional or committed loader.
> 
> I don’t understand that sentence well enough. The provisional state is used before we commit. It would be a problem if we got stuck in the provisional state.

I don't think we should ever get stuck in the provisional state, because we will commit in DocumentLoader::finishedLoading() if we haven't already (this is called indirectly from MainResourceLoader::didFinishLoading()).

It appears what's happening in the original code is that multipart content is handled in a special way from other main resources.  Normally, we just commit the load as soon as we receive the first byte.  The  multipart content logic, on the other hand, is sufficiently complex that I had to write myself some pseudocode to make sense of it:

if (this is the first part)
    load normally
else if (the mime type is text/html)
    revert to provisional and load normally
else {
    leave committed;

    if (this is the last part)
        ??? I can't find where we commit the data.
    else {
        wait until we receive a response for the
        next part, then revert to provisional and
        immediately commit all of this part's data
    }   
}

Hopefully this is easier to follow than my last explanation.

I can't immediately see a reason for this for this discrepancy between handling regular and multipart content, but I'm probably missing something.

[Darin Adler]

Did you test multi-part content?

[Nate Chapin]

(In reply to comment #11)
> Did you test multi-part content?

Only in layout test form.  I will happily accept suggestions for good multipart content to test manually :)

[Darin Adler]

To find examples, you need to search for multipart/x-mixed-replace on the web. I’m looking now for test cases.

[Eric Seidel (no email)]

Curious what the status is here, nate?

[Nate Chapin]

Created attachment 76585 [details]
Swap calls to writer()->end() and setupForReplace() in DocumentLoader

Eric pointed me at some good resources for testing this content at http://www.opentopia.com/hiddencam.php, which revealed a consistent crash.

writer()->end() needs to be before the call to setupForReplace(), because setupForReplace() reverts the FrameLoader to provisional state, and writer()->end() is assuming that the FrameLoader is committed.

Note that to trigger this codepath, you need to open one of the webcam feeds in its own tab (i.e., as a main resource).

[Eric Seidel (no email)]

That link isn't nearly as shady as it sounds. :)  It just happened to be one of the top hits when searching for public webcam. :)  IIRC networkable webcams commonly use multi-part mixed replace.  Clearly we need more testing coverage here.

[Eric Seidel (no email)]

Comment on attachment 76585 [details]
Swap calls to writer()->end() and setupForReplace() in DocumentLoader

If you found a crash, we need to test that crash.  The tests are more important than code changes in most case.

[Eric Seidel (no email)]

Comment on attachment 73080 [details]
Move DocumentWriter call back to DocumentLoader

Cleared Adam Barth's review+ from obsolete attachment 73080 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Nate Chapin]

Created attachment 164191 [details]
patch + test

This patch is in the same theme of the one I wrote 2 years ago, but the low hanging fruit in the referenced functions has changed almost completely since then.

See WebCore/ChangeLog for a full list of the changes, but note that this does modify behavior in the case of text/html multipart pieces.  Our behavior in that case appears to date back to WebKit's initial multipart/x-mixed-replace support in trac.webkit.org/changeset/10489, but I can't find a rationale for it.  The behavior of this patch seems better to me (i.e., the document isn't blank).

[Eric Seidel (no email)]

Comment on attachment 164191 [details]
patch + test

View in context: https://bugs.webkit.org/attachment.cgi?id=164191&action=review

OK.  I've highlighted all the code changes below.  The rest looks like just shuffling lines but no functional change.  If you could explain those in greater detail (again?) that will make this easier to review.  Thanks.

> Source/WebCore/loader/DocumentLoader.cpp:335
> -        if (!m_mainResourceLoader || !m_mainResourceLoader->isLoadingMultipartContent() || !frameLoader()->isReplacing())
> +        // for multipart loads, and FrameLoader::isReplacing() will be true after the first time.
> +        if (!isMultipartReplacingLoad())

Why is removing !m_mainResourceLoader OK here?  Was it a no-op?

> Source/WebCore/loader/DocumentLoader.cpp:381
> -    if (doesProgressiveLoad(m_response.mimeType()))
> +    if (!isMultipartReplacingLoad())

Previously this could happen even when loading multipart content, correct?  But now this if doesn't allow that?

> Source/WebCore/loader/DocumentLoader.cpp:-420
> -    if (doesProgressiveLoad(newMIMEType)) {
> -        frameLoader()->client()->revertToProvisionalState(this);
> -        setupForReplace();
> -    }
> -    

Could you explain this removal?  Is this a functional change?

[Nate Chapin]

(In reply to comment #20)
> (From update of attachment 164191 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164191&action=review
> 
> OK.  I've highlighted all the code changes below.  The rest looks like just shuffling lines but no functional change.  If you could explain those in greater detail (again?) that will make this easier to review.  Thanks.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:335
> > -        if (!m_mainResourceLoader || !m_mainResourceLoader->isLoadingMultipartContent() || !frameLoader()->isReplacing())
> > +        // for multipart loads, and FrameLoader::isReplacing() will be true after the first time.
> > +        if (!isMultipartReplacingLoad())
> 
> Why is removing !m_mainResourceLoader OK here?  Was it a no-op?

isMultipartReplacingLoad() calls DocumentLoader:isLoadingMultipartContent(), rather than directly calling MainResourceLoader:isLoadingMultipartContent(). The DocumentLoader helper null-checks m_mainResourceLoader.

> 
> > Source/WebCore/loader/DocumentLoader.cpp:381
> > -    if (doesProgressiveLoad(m_response.mimeType()))
> > +    if (!isMultipartReplacingLoad())
> 
> Previously this could happen even when loading multipart content, correct?  But now this if doesn't allow that?

If I understand this comment correctly...
Per the ChangeLog, this is the known functional change. Previously, multipart loads would only be treated as progressive if they were loading text/html. I removed the text/html special case, which makes doesProgressiveLoad() equivalent to !frameLoader()->isReplacing().

However, it's not *quite* that easy. Chromium will, for error pages, occasionally force a non-multipart load to be replacing. In this case, it still wants progressive loading, hence the multipart AND replacing.

The inverse case, multipart and not replacing, also happens for the first part of a multipart resource, as setReplacing() is not called until the second response is received for the multipart load. I'm not sure this case particularly matters whether it's loaded progressively or not, but I can look more into it.


> 
> > Source/WebCore/loader/DocumentLoader.cpp:-420
> > -    if (doesProgressiveLoad(newMIMEType)) {
> > -        frameLoader()->client()->revertToProvisionalState(this);
> > -        setupForReplace();
> > -    }
> > -    
> 
> Could you explain this removal?  Is this a functional change?

Remember that, because text/html is no long special-cased as progressive,  doesProgressiveLoad() is synonymous with !isReplacing(). We just called setReplacing() 2 lines above this point and !isReplacing() is therefore guaranteed to be false here, so this if statement should never be called.

[Nate Chapin]

In case it's helpful, here's a summary of what each browser does in the case of multipart/x-mixed-replace resources with text/html part:

IE9: Appends each text/html part to the document, no formatting or parsing at all, no replacing happens.
Opera: Doesn't navigate to the multipart resource, remains on previous page.
FF: Replaces, displays content as text/html properly.
WebKit: Navigates to multipart resource, but shows a blank page.

WebKit's behavior matches FF after this patch.

[Eric Seidel (no email)]

Comment on attachment 164191 [details]
patch + test

Thank you.  This LGTM.  You may wish to wait 24 hrs for other reviewers to offer their thoughts.

[Darin Adler]

How did you test these changes? Did you test them in Chromium only, or on other platforms too?

[Nate Chapin]

(In reply to comment #24)
> How did you test these changes? Did you test them in Chromium only, or on other platforms too?

I'm pretty sure I tested it on safari mac as well, but that was a couple weeks ago. I'll double check tomorrow before landing.

[Nate Chapin]

(In reply to comment #25)
> (In reply to comment #24)
> > How did you test these changes? Did you test them in Chromium only, or on other platforms too?
> 
> I'm pretty sure I tested it on safari mac as well, but that was a couple weeks ago. I'll double check tomorrow before landing.

I verified that all multipart layout tests still pass, and the new test looks right to me in a full safari debug build.

I intended to manually verify that multipart images still work correctly, but unlike 2 years ago, I've had considerable difficulty finding a public webcam that exposes its data as multipart/x-mixed-replace. All the streams I found either used flash or static jpgs + refresh.

[Eric Seidel (no email)]

(In reply to comment #15)
> Created an attachment (id=76585) [details]
> Swap calls to writer()->end() and setupForReplace() in DocumentLoader
> 
> Eric pointed me at some good resources for testing this content at http://www.opentopia.com/hiddencam.php, which revealed a consistent crash.

I think these still work. The first cam I clicked on looked like it might be multipart/mixed?  I'm not sure how to tell:

http://89.200.82.159:8001/axis-cgi/mjpg/video.cgi?resolution=640x480&clock=0&date=0

[Nate Chapin]

(In reply to comment #27)
> (In reply to comment #15)
> > Created an attachment (id=76585) [details] [details]
> > Swap calls to writer()->end() and setupForReplace() in DocumentLoader
> > 
> > Eric pointed me at some good resources for testing this content at http://www.opentopia.com/hiddencam.php, which revealed a consistent crash.
> 
> I think these still work. The first cam I clicked on looked like it might be multipart/mixed?  I'm not sure how to tell:
> 
> http://89.200.82.159:8001/axis-cgi/mjpg/video.cgi?resolution=640x480&clock=0&date=0

Awesome, that is multipart/x-mixed-replace according to chromium's net-internals. Both chromium mac and safari mac handle it correctly before and after my patch.

[WebKit Review Bot]

Comment on attachment 164191 [details]
patch + test

Clearing flags on attachment: 164191

Committed r130651: <http://trac.webkit.org/changeset/130651>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.