339610 - caller can't know when a multipart/x-mixed-replaced stream is done

Status: NEW

(Core :: Networking, defect, P5)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Right now it's impossible for a user of the network library to know that a multipart/x-mixed-replace stream is done.  Currently the caller gets a series of onStartRequest and onStopRequest pairs (one pair for each part).  Depending on how the stream ends, nsIMultiPartChannel::isLastPart may be set to true for the last part, but this isn't guaranteed since the stream may close sometime after the last onStopRequest without another part coming.

This turned out to be a problem for bug 321054.

Boris suggested in bug 321054 comment 40:
# Please file a followup bug on the multipart converter? It should probably
# just not send OnStartRequest until either it's ready for the OnStartRequest
# for the next part or is done completely; that way it's guaranteed to get
# the "last part" boolean correct.
This seems like it might help, although it might delay onStopRequests that we'd be better off knowing about sooner.

The other related issue is that not all callers may be expecting multiple pairs of onStartRequest and onStopRequest notifications, and perhaps multipart/x-mixed-replace shouldn't be handled so transparently by necko.  If it were handled less transparently (and callers had to do something to handle it, like add an adaptor object as the stream listener), it would be easier for callers to set up listeners for both the stream as a whole (if they need it) and the individual parts.  I had an IRC discussion with darin about this -- I'll see if I can find the log.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Once this is fixed the chunk of code I'm adding to nsImageLoadingContent::DestroyImageLoadingContent in bug 321054:

  // This can actually fire for multipart/x-mixed-replace, since if the
  // load is canceled between parts (e.g., by cancelling the load
  // group), we won't get any notification.  See bug 321054 comment 31
  // and bug 339610.  *If* that multipart/x-mixed-replace image has
  // event handlers, we won't even get to this warning; we'll leak
  // instead.
  NS_WARN_IF_FALSE(mRootRefCount == 0,
                   "unbalanced handler preservation refcount");
  if (mRootRefCount != 0) {

should be able to be changed to an NS_ASSERTION.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> although it might delay onStopRequests that we'd be better off knowing about 
> sooner.

This is unfortunately true.  :(

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: IRC discussion about this issue

Comment 4

[Boris Zbarsky [:bzbarsky]]

So thing is, the stream converter does NOT happen automagically now.  It's only created for loads in docshells (in the URILoader), in imagelib (in imgLoader), for XMLHttpRequest, and in the mailnews URL fetcher.

So imagelib _does_ have access to the base channel in ProxyListener::OnStartRequest (which is where it creates the converter).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Once this is fixed, the code being added in bug 373701 can be simplified a good bit.

Comment 6

[Bobby Holley (:bholley)]

Is anyone with the know-how to fix this available to do so in the next few weeks? If not, can someone direct me how to do it myself? I'm overhauling a lot of stuff in imagelib and really need this fixed to clean up imgRequest and the decoderobserver notification system. Not sure if darin is still actively maintaining necko stuff.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Bobby, I think all the information that's readily available on this is already in the bug.  It's not quite clear how best to fix this, which is why it hasn't happened yet.

Darin is not actively working on Necko.

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 10

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=3 years untouched DOM/Storage bug's priority.

If you have reason to believe this is wrong, please write a comment and ni :jstutte.