1612992 - view-source doesn't load with mutipart/x-mixed-replace

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Matt Woodrow (:mattwoodrow)]

Load view-source:https://xenon.coheris.com/ff73/

Crashes in debug builds with 'Assertion failure: !mPartChannel (tisk tisk, shouldn't be overwriting a channel), at /Users/mwoodrow/src/mozilla-central3/netwerk/streamconv/converters/nsMultiMixedConv.cpp:822'

Opt builds just fail to finish loading.

Comment 1

[Matt Woodrow (:mattwoodrow)]

The issue here is that we have nsMultiMixedConv and then nsViewSourceChannel as listeners to the nsHttpChannel in the parent.

nsViewSourceChannel::OnStartRequest gets called with aRequest being an nsPartChannel instance. We then replace the request with
|this| and forward onto ParentChannelListener::OnStartRequest.

Since aRequest is no longer an nsIMultiPartChannel (since it's nsViewSourceChannel), ParentChannelListener doesn't realize that we're multi-part.

Once the first part ends, ParentChannelListener clears mNextListener (not expecting further parts), and then the next part triggers errors and crashes.

Possible fixes:

• Get nsMultiMixedConv to be a listener after nsViewSourceChannel instead of before. I'm not sure how to implement this, since nsHttpChannel is what installs the multipart converter.

• Get nsViewSourceChannel to conditionally implement nsIMultiPartChannel, and make it copy the values from the aRequest passed to OnStartRequest.

Any thoughts Honza?

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)

• Get nsViewSourceChannel to conditionally implement nsIMultiPartChannel, and make it copy the values from the aRequest passed to OnStartRequest.

This option seems better to me, we will mostly copy the existing pattern to make v-s channel just forward to the providing inner channel.

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Honza Bambas (:mayhemer) from comment #2)

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)

• Get nsViewSourceChannel to conditionally implement nsIMultiPartChannel, and make it copy the values from the aRequest passed to OnStartRequest.

This option seems better to me, we will mostly copy the existing pattern to make v-s channel just forward to the providing inner channel.

This unfortunately doesn't work either :(

We get multiple OnStartRequest calls cached by DocumentLoadListener, and they all have the same nsIRequest* value (the nsViewSourceChannel instance).

Then when we pass them to HttpChannelParent and query for nsIMultiPartChannel::isLastPart they all they return true (since the inner channel in nsViewSourceChannel is now the last part).

I think we really need a unique nsIRequest* forwarded on for each part.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1612992 - Don't replace the nsIRequest passed to OnStart/StopRequest with nsViewSourceChannel if we don't need it. r?mayhemer

Comment 5

[Honza Bambas (:mayhemer)]

I don't follow the problem as you describe it comment 3. Can you please outline some kind a simple diagram how the objects link?

Thanks and sorry for late reply.

Comment 6

[Matt Woodrow (:mattwoodrow)]

The flow is:

nsHttpChannel -> nsMixedMultiConv -> nsViewSourceChannel -> ParentChannelListener -> DocumentLoadListener -> HttpChannelParent.

The problem is that the step from DocumentLoadListener to HttpChannelParent isn't synchronous, and instead we cache the nsIRequest in DocumentLoadListener::mStreamListenerFunctions.

In the case where we get multiple parts, and multiple OnStartRequests called, there multiple be multiple entries in mStreamListenerFunctions, but all with the same nsIRequest* (the nsViewSourceChannel).

When we do forward these on to HttpChannelParent, it does a QI to nsIMultiPartChannel (successful, since the idea was that we implemented that for nsViewSourceChannel). It then checks for isLastPart, which will be true for the first part (and all parts), since they're all the same instance.

Comment 7

[Honza Bambas (:mayhemer)]

Thanks! Way more clear now.

Isn't it in reality even a bit more like:
nsHttpChannel -> nsMixedMultiConv -> nsPartChannel -> nsViewSourceChannel -> ParentChannelListener -> DocumentLoadListener -> HttpChannelParent.

So the idea behind the patch is that aRequest given to HttpChannelParent is NOT nsViewSourceChannel but it IS the particular nsPartChannel, which remembers the correct lastPart flag. Got it.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07bf7a599bce
Don't replace the nsIRequest passed to OnStart/StopRequest with nsViewSourceChannel if we don't need it. r=mayhemer

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/07bf7a599bce