Closed Bug 1612992 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(1 file)

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.

See Also: → 1611081
Regressed by: 1600211
Has Regression Range: --- → yes
Keywords: regression

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?

Flags: needinfo?(honzab.moz)

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

Flags: needinfo?(honzab.moz)
Priority: -- → P2
Whiteboard: [necko-triaged]

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

Assignee: nobody → matt.woodrow
Status: NEW → ASSIGNED

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.

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

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.

Pushed by mwoodrow@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Regressions: 1631782
You need to log in before you can comment on or make changes to this bug.