733553 - FF keeps loading and continuously calls "onerror" handler on multipart image stream when image in stream changes size

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Kabs, reporting bugs since 2002]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

FF displays a multipart/x-mixed-replaced stream containing jpeg images. 

Stream of JPEG images changes image size.


Actual results:

- Image is not displayed any more.
- ALT text is displayed instead
- FF continues to load the image stream in background
- FF calls "onerror" handler for every image loaded.


Expected results:

- Image should be displayed with new and correct image size

Comment 1

[Daniel Kabs, reporting bugs since 2002]

Testcase will follow soon.

Comment 2

[Daniel Kabs, reporting bugs since 2002]

Attachment: Testcase: instructions inside!

Comment 3

[Daniel Kabs, reporting bugs since 2002]

Also reproducible on MacOs.

Comment 4

[Daniel Kabs, reporting bugs since 2002]

Remark: the image size is changed on the server (i.e. on the webcam) side.

Comment 5

[Adam]

I can reproduce this (using 11 beta and 13 nightly) on Linux.  

Firefox should stop loading a bad stream if it can't recover (Daniel tried reverting the image size and Firefox didn't recover).  Whether it should try to transition over a change in the stream is debatable.

Comment 6

[Marcia Knous [:marcia]]

I am able to reproduce this given the instructions in the test case using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0.2) Gecko/20100101 Firefox/10.0.2. I get the "image corrupt or truncated" error in the console.

Comment 7

[Tanner Filip [:tanner]]

I can also reproduce on Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120306 Firefox/13.0a1.

Comment 8

[Adam]

Digging around a bit, the source of ceasing to decode the stream comes in image/src/RasterImage.cpp:SetSize (~1130).  There is a check if it already had a size, and if so, did it change. 

If it did change, it places the container in the error state, which means subsequent data just retriggers the onerror condition.

Without that check, it segfaults in image/src/imgFrame.cpp:UnlockImageData (~658) checking mPalettedImageData.  Haven't found exactly why, but mPalettedImageData is size-dependent, so changing the size is somehow impacting the previous frame's expected allocation?

Comment 9

[Joe Drew (not getting mail)]

We re-use the image frame when we're in the multipart case (RasterImage::EnsureFrame, called from the JPEG decoder), but only if it's the same size.

What's the backtrace from UnlockImageData?

Comment 10

[Daniel Kabs, reporting bugs since 2002]

Adam, thanks for the analysis. Indeed FF had crashed in earlier versions when size of images in stream changed, see bug #639303

Comment 11

[Adam]

Attachment: Backtrace for UnlockDataImage upon disabling the RasterImage SetSize check.

To be clear, this isn't a segfault from the regular code, but what happens when you disable the error for multiparts in RasterImage.cpp:SetSize().

Comment 12

[Joe Drew (not getting mail)]

I suspect what's happening here is that the frame is getting deleted (and set to NULL - you can see that in the backtrace at UnlockImageData, this = 0x0 or NULL). I'd have to do more debugging to figure out why.

Adam, if you're up to debugging this, you'll want to look to see what nsJPEGDecoder does while it's decoding. SetSize will be called from there.

Comment 13

[Adam]

Attachment: WIP v0: Allow the resize to take place, properly clear the frames. (Doesn't actually resize in content, just scales)

This patch works, except that it is scaling the resized images to whatever size the stream was initially, rather than resizing the image.  There must be a container that needs to either be replaced or resized to allow the content reflow/the actual layout image size to change.  

It looks like other stream changes should be okay, as they rely on separate decoders for each part, but I haven't tested that (still looking for an easy way to create a dummy stream locally for such testing).

Comment 14

[Daniel Kabs, reporting bugs since 2002]

Adam, that's great news. Hope you could put to good use the stream of the cam in the testcase.

Did you notice that a plain multipart JPEG stream (i.e. not embedded in a web page) does not exhibit the issue described here? See this URL for example (credentials as in the testcase):

http://appcam.mobotixcam.de:2080/control/faststream.jpg?stream=full&fps=0

Comment 15

[Adam]

Attachment: Simple Python multipart streamer

Here's a small python multipart streamer that can vary image formats in the same stream for debugging.  Along with Daniel's test case this should help debugging the variety of possibilities for these multipart streams.

Using it requires a webpage referencing the 'http://localhost:8080' image as the src (sample markup provided in the source), and it requires configuring the images to use (defaults to 'testfile0.png' and 'testfile1.jpg' in the same directory as the streamer).

Thanks for mentioning the stand-alone images, Daniel, I confirmed your result.  In doing so I found that stand-alone viewing handles format changes as well.  In-page images don't work with format changes (though parts that match the initial format are still used, mismatched parts cause Decoder.cpp::Finish() to emit "Image corrupt or truncated" in the error console).

Comment 16

[Adam]

Attachment: WIP v1: Allow content type changes.

This will now allow raster-to-raster content type changes on in-page image streams.  

Doesn't yet resize the image in layout (so size changes still just scale into the original footprint).  Also haven't yet tried to deal with vector images in streams.

Comment 17

[Adam]

Attachment: WIP v2: Let the size change via layout/generic/nsImageFrame

This patch lets the images resize, though it doesn't support switching between vector and raster formats.  Letting the resize occur can also be improved upon.

The problem is that nsImageFrame::OnStopDecode only resizes if the request is a pending request, which is decided in nsImageLoadingContent::PrepareNextRequest.  But for a multipart stream, only the initial request is used, and it's a current request.  

This patch just removes the distinction between current and pending requests for OnStopDecode, but it'd might be better to find a way to check if it's a multipart there.  Even then, every new part of the stream will still check for a resize, even if none occurred.  Testing the patch a bit didn't show an unreasonable number of extra calls occurring.

Comment 18

[Adam]

Attachment: Updated multipart test server to support SVGs.

This is a small update to support SVGs.  They aren't resized by the server, so using a couple of different-sized SVGs for testing would be best.  Default names are testfile{2,3}.svg

Comment 19

[Adam]

Attachment: multipart test server: Remove extraneous newline.

Comment 20

[Adam]

Attachment: WIP v3: Allow format changes between Raster/Vector

I believe this does everything needed on the image side, but the stream often holds the very last chunk of data until the next part has already arrived.  

That delays the OnStopDecode until a new part is ready to begin decoding, meaning that the size of the image only changes when a new part (which may have a different size itself) is loading.  Furthermore, SVGs do not have a "partial load", so they may not even appear until a new part is ready to take their place.

The current version of the test server tries to work around that by always sending the close token ('--boundary--') after each part, but this causes intermittent data errors for the decoders.  When they don't get data errors, the decode completes properly and the size is updated for that current part.

Comment 21

[Adam]

Attachment: WIP v4: Fix bug with mime detection, improve nsMultiMixedConv's behavior

The re-setting of the mime type could happen too early in imgRequest.cpp, so it now happens again closer to the initialization of the new decoder.  This prevents the corruption issues I mentioned in comment 20.

This also changes nsMultiMixedConv.cpp to send a stop when it sends all of the remaining data at the end of its OnDataAvailable().  Currently this only helps when the parts end either with the end delimiter for multipart, or if they end in a newline.  I'm not yet sure whether that can be expanded to successfully detect the end of a part if it's not marked (in order to send the stop).

Also noticed that for some yet undetermined reason if the same vector is sent in succession then the second-plus instances don't display.  Haven't tracked down why yet.

Comment 22

[Adam]

Attachment: WIP v5: Send as much data as possible from nsMultiMixedConv::OnDataAvailable

Okay, so I was wrong about being able to send stop if the data ended in a newline.  Unfortunately the best we can do is close to what was already: we can send a little more data if we determine no partial token match in the remaining data in nsMultiMixedConv::OnDataAvailable.  That's implemented by a new nsMultiMixedConv::FindPartialToken that looks to see if any of the remaining data could begin a new token and sending what it can exclude (rather than holding up to a token's length of data).

That does result in the decode finishing, but that can't trigger a resize (at least until bug 505385 is complete) until the next part (ending the previous part's request).

Also, I get a crash when the first part on a fresh start is a vector image.  This happens after VectorImage::OnStopRequest notifies the observer that size is known via OnStartContainer.  Partial trace:  

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff551951f in nsRefPtr (this=0x7fffffff8ee0)
    at ../../dist/include/nsAutoPtr.h:914
914	            : mRawPtr(0)

#0  0x00007ffff551951f in nsRefPtr (this=0x7fffffff8ee0)
    at ../../dist/include/nsAutoPtr.h:914
#1  mozilla::image::SVGDocumentWrapper::GetWidthOrHeight (
    this=<optimized out>, 
    aDimension=mozilla::image::SVGDocumentWrapper::eWidth, 
    aResult=@0x7fffffff8f98: 0)
    at ./mozilla-central/image/src/SVGDocumentWrapper.cpp:111
#2  0x00007ffff55199c2 in GetWidth (aWidth=0x7fffffff8f98, 
    this=<optimized out>)
    at ./mozilla-central/image/src/VectorImage.cpp:327
#3  mozilla::image::VectorImage::GetWidth (this=<optimized out>, 
    aWidth=0x7fffffff8f98)
    at ./mozilla-central/image/src/VectorImage.cpp:319
#4  0x00007ffff55cb315 in nsImageFrame::UpdateIntrinsicSize (
    this=0x7fffd88f2e00, aImage=0x7fffda8d5ba0)
    at ./mozilla-central/layout/generic/nsImageFrame.cpp:335
#5  0x00007ffff55ccd8c in EnsureIntrinsicSizeAndRatio (
    aPresContext=<optimized out>, this=0x7fffd88f2e00)
    at ./mozilla-central/layout/generic/nsImageFrame.cpp:750

Still looking into the cause here.

Comment 23

[Adam]

Attachment: WIP v6: All is well

Digging into the last crash got me to slough off some bad assumptions.  I got the same crash without the patch.  Looking at bug 276431 where Vectors got added to the multipart mix, it's likely they never worked.

This patch drops the nsMultiMixedConv change the previous patch had, which turned out to be unneeded.  It also removes attempts to make VectorImage be multipart like Raster is (in light of the above), instead favoring replacing the whole image both between Vectors and at Raster/Vector transitions.

The only thing I don't like about this version of the patch is that images retrain their predecessor's size briefly, but that is caused by the inability of the decoder to notify the image frame.  Bug 505385 should allow that to be remedied with an OnSizeAvailable load event.

Comment 24

[Robert Lickenbrock [:rclick]]

(In reply to Adam [:hobophobe] from comment #23)
> The only thing I don't like about this version of the patch is that images
> retrain their predecessor's size briefly, but that is caused by the
> inability of the decoder to notify the image frame.  Bug 505385 should allow
> that to be remedied with an OnSizeAvailable load event.

OnStartContainer is what you want here. In the current world it fires when the image's size becomes available (specifically, at [1] for raster images and at [2] for vector images).

[1] https://hg.mozilla.org/mozilla-central/file/babbc38b7f52/image/src/Decoder.cpp#l234
[2] https://hg.mozilla.org/mozilla-central/file/babbc38b7f52/image/src/VectorImage.cpp#l709

Comment 25

[Adam]

Attachment: WIP v7: Allow imgRequestProxy to send OnStartContainer once per part for multipart

Thanks for the hint about OnStartContainer, rclick!  imgRequestProxy was only sending OnStartContainer once per proxy, which meant new parts couldn't update their size early, only after they had decoded.  This resets the mSentStartContainer flag for multipart requests in imgRequestProxy::OnStopContainer.

Now need to tackle animated GIFs (which currently segfaults when the image's size is being calculated via RasterImage.cpp's SizeOfDecodedWithComputedFallbackIfHeap, due to trying to size a null frame).  

Chromium plays the animation until a new part replaces it.

Comment 26

[Adam]

Attachment: WIP v8: Adds mMultipartFresh flag to aid in handling animated RasterImages

This uses mMultipartFresh for two purposes.  First, in RasterImage::AdvanceFrame to determine that the current decoder isn't relevant (it's sitting ready for the next part).  Second, it's used in RasterImage::AddSourceData to let us clean up after the previous part for the first incoming data on a new part.  It cleans up any animation and frames from that.

The standalone view of an image stream (ie, content/html/document/src/ImageDocument.cpp) would need some changes to support the refinements here , but that may be better left to a followup.  

For what it's worth, in that case the OnStartRequest for a future/pending part actually replaces the document before the image is ready.  The result is that most of time the URL of the image stream is displayed (it's the alt text and displays until the image loads).  The exception is when the new part is SVG, which I believe is displayed as a document in stand-alone mode, rather than as an image.  That causes the part to be skipped (not entirely clear where/why that occurs), so that the previous part is allowed to stay displayed.

Comment 27

[Adam]

Attachment: Updated multipart test server to handle animated images

Forgot to add an updated copy that handles animated Raster images.

I'm including test images so if anyone else wants to use this they don't have to dig any up.

Comment 28

[Daniel Kabs, reporting bugs since 2002]

Adam, thanks for helping and creating a patch.

How do things proceed from here? Will the patch be included in the nightly build after passing review?

Comment 29

[Joe Drew (not getting mail)]

Comment on attachment 614944 [details] [diff] [review]
WIP v8: Adds mMultipartFresh flag to aid in handling animated RasterImages

Review of attachment 614944 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the long delay before reviewing.

Is there a cleaner way we could cause reinitialization (rather than adding another bit of state)? I'm not overwhelmingly happy about that.

r- is for answering a few questions, though.

::: image/public/imgIRequest.idl
@@ +134,5 @@
>  
>    /**
> +   * Whether the request is multipart (ie, multipart/x-mixed-replace)
> +   */
> +  readonly attribute bool multipart;

You need to rev imgIRequest's uuid.

::: image/src/RasterImage.cpp
@@ +1092,5 @@
>      imgFrame *prevframe = mFrames.ElementAt(mFrames.Length() - 1);
> +    if (prevframe) {
> +      prevframe->UnlockImageData();
> +    } else if (mMultipart) {
> +      mFrames.Clear();

I don't understand this part. Can you explain?

::: image/src/imgRequest.cpp
@@ -774,5 @@
> -    } else {  // imageType == imgIContainer::TYPE_VECTOR
> -      nsCOMPtr<nsIStreamListener> imageAsStream = do_QueryInterface(mImage);
> -      NS_ABORT_IF_FALSE(imageAsStream,
> -                        "SVG-typed Image failed QI to nsIStreamListener");
> -      imageAsStream->OnStartRequest(aRequest, ctxt);

This change means we no longer call OnStartRequest in this case. Is that correct?

Comment 30

[Adam]

Attachment: v9: Remove extra state in favor of an existing equivalent; address other review concerns.

Thanks Joe!

This patch replaces |mMultipartFresh| with checks if |mDecodedBytes| is zero, to avoid adding that extra state unnecessarily.

Revised the UUID for imgIRequest.idl.

> ::: image/src/RasterImage.cpp
> @@ +1092,5 @@
> >      imgFrame *prevframe = mFrames.ElementAt(mFrames.Length() - 1);
> > +    if (prevframe) {
> > +      prevframe->UnlockImageData();
> > +    } else if (mMultipart) {
> > +      mFrames.Clear();
> 
> I don't understand this part. Can you explain?

|EnsureFrame| looks to reuse the frame, but only if it matches the new frame's details.  When it doesn't match, |EnsureFrame| deletes the frame and then calls |InternalAddFrame|.  That's the case which leads to the part you're asking about.

At that point, that frame is gone, but |mFrames| still has a length of one, which means it tries to to call |UnlockImageData| on the non-existent frame.  We avoid that and clear |mFrames|.

This patch adds a comment there to clarify.

> ::: image/src/imgRequest.cpp
> @@ -774,5 @@
> > -    } else {  // imageType == imgIContainer::TYPE_VECTOR
> > -      nsCOMPtr<nsIStreamListener> imageAsStream = do_QueryInterface(mImage);
> > -      NS_ABORT_IF_FALSE(imageAsStream,
> > -                        "SVG-typed Image failed QI to nsIStreamListener");
> > -      imageAsStream->OnStartRequest(aRequest, ctxt);
> This change means we no longer call OnStartRequest in this case. Is that correct?

We call it, just not directly.  We defer to |OnDataAvailable| to replace the image.  |OnDataAvailable| is where |mImage| gets created originally, so this just lets it take care of that for each creation in multipart.  As part of that, it calls either |OnStartRequest| (for vector images) or |RequestDecode| (for raster images).

Comment 31

[Joe Drew (not getting mail)]

(In reply to Adam [:hobophobe] from comment #30)
> |EnsureFrame| looks to reuse the frame, but only if it matches the new
> frame's details.  When it doesn't match, |EnsureFrame| deletes the frame and
> then calls |InternalAddFrame|.  That's the case which leads to the part
> you're asking about.
> 
> At that point, that frame is gone, but |mFrames| still has a length of one,
> which means it tries to to call |UnlockImageData| on the non-existent frame.
> We avoid that and clear |mFrames|.
> 
> This patch adds a comment there to clarify.

Aha!

I think we should change that code, then. It's clear that this doesn't happen in the real world, because we'd have null deref crashes.

We should probably replace the call to InternalAddFrame with InternalAddFrameHelper, since that simply creates a new frame and puts it into the appropriate location in the array. Then we don't need the confusing code at all.

Comment 32

[Adam]

Attachment: v10: Replace an unreusable frame directly from EnsureFrame via InternalAddFrameHelper.

Comment 33

[Joe Drew (not getting mail)]

Comment on attachment 622142 [details] [diff] [review]
v10: Replace an unreusable frame directly from EnsureFrame via InternalAddFrameHelper.

Review of attachment 622142 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray!!

::: image/src/RasterImage.cpp
@@ +1214,2 @@
>    DeleteImgFrame(aFrameNum);
> +  mFrames.RemoveElementAt(aFrameNum);

Arguably, you could move this into the body of DeleteImgFrame(). "Listen to your heart"

Comment 34

[Adam]

(In reply to Joe Drew (:JOEDREW!) from comment #33)
> ::: image/src/RasterImage.cpp
> @@ +1214,2 @@
> >    DeleteImgFrame(aFrameNum);
> > +  mFrames.RemoveElementAt(aFrameNum);
> 
> Arguably, you could move this into the body of DeleteImgFrame().

I pondered that when working on the last patch.  If there were more callers, or if the code were more critical, it would be a clearer decision.  The only two callers are:

1. EnsureFrame(), which would be only marginally cleaner.
2. AddSourceData() (clearing out the previous part's animation frames), which would need its loop reversed and would have the array shrink over the course of the loop instead of all at once.

Given its the limited callers (therefore limited messiness), and that [2] does see some upside in using Clear(), I'm okay with keeping it as-is.

> "Listen to your heart"

[Aside: One day in Kindergarten the student teacher brought in a stethoscope so that the kids could hear their heartbeats.  In turn, each student would go sit next to her.  She would find their heartbeat, then let them listen.

Finally my turn came.  I was eager to hear the thumping, excited by the wild imitations the others in the class performed after  hearing their own hearts.

Despite a good minute's search, even retesting the stethoscope on herself, she came up empty.  Alas, she could not find my heartbeat.]

Comment 35

[Joe Drew (not getting mail)]

Adam, if you add a testcase that we could run in an automated fashion, I could check that in too (and it would be *awesome*). We have a few multipart/x-mixed-replace testcases; one such is image/test/reftest/jpeg/webcam-simulacrum.mjpg (there's instructions on how I made it in reftest.list; I did that sort of hacky thing because I couldn't get proper multipart/x-mixed-replace working with httpd.js, though maybe you'd have more luck).

https://hg.mozilla.org/integration/mozilla-inbound/rev/501d38d3892c

Comment 36

[Joe Drew (not getting mail)]

Unfortunately I had to back this out for causing crashes in xpcshell tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf1739a4e17

Comment 37

[Adam]

Attachment: Existence check before deref on imgRequestProxy::mOwner

Sorry about that, Joe.  I've fixed that pair of crashes.

I'll get a test for the rest of the changes written soon, so if you'd prefer to wait for checkin until I have those, that's okay too.

Comment 38

[Joe Drew (not getting mail)]

In the mean time, let's see what Try has to say about this.

Comment 39

[Adam]

Attachment: Test multipart image content with size and type changes.

This adds a mochitest-plain test which sets up a multipart response using httpd.js.  The server part of the patch is bug733553.sjs.

On the test-side (test_bug733553.html), each part loading causes a load event on the image element, and the event handler runs a test to see that the loaded part's width is correct.

Comment 40

[Joe Drew (not getting mail)]

Comment on attachment 622575 [details] [diff] [review]
Test multipart image content with size and type changes.

Review of attachment 622575 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! I'm really glad you got a real multipart/x-mixed-replace test, rather than my silly hacky version.

::: image/test/mochitest/bug733553.sjs
@@ +32,5 @@
> +  fileStream.init(file, 1, 0, false);
> +  var binaryStream = Components.classes['@mozilla.org/binaryinputstream;1']
> +                     .createInstance(Components.interfaces.nsIBinaryInputStream);
> +  binaryStream.setInputStream(fileStream);
> +  return [binaryStream, fileStream];

Why do you return both the binary stream and the file stream?

::: image/test/mochitest/test_bug733553.html
@@ +17,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +var testIndex = 0;
> +var testWidths = [1, 40, 1, 100, 3000, 40, 1];

You might want to document which images these widths come from.

@@ +28,5 @@
> +
> +function imageLoad(aEvent) {
> +  if (testWidths.length > testIndex) {
> +    is(aEvent.target.width, testWidths[testIndex++],
> +       "Test " + (testIndex - 1) + " width not correct");

is aEvent.target.src set too? That'd be even more informative if we get a failure.

Comment 41

[Adam]

Attachment: Include filenames in test output, skip unneeded binary input stream in favor of the file input stream.

> > +  return [binaryStream, fileStream];
> 
> Why do you return both the binary stream and the file stream?

A faulty assumption that it needed to be closed separately.  Closing the binary stream would close the file stream, so it wasn't necessary.  

Turns out that we don't need the binary input stream at all, so this just returns the file stream now.

> is aEvent.target.src set too?  

It always points to its initial src, bug733553.sjs, as the src doesn't change for multipart.

Comment 42

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 622270
	Branch: mozilla-central => try
Insufficient permissions to push to try.

Comment 43

[Adam]

Looks like the bot didn't want to help us, so I'm requesting check-in.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b73ef4b11140
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8a314452a2

Thanks for the patch!

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, that Try push would have been useful. Backed out due to mochitest crashing on the new test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9691915b4c0d

https://tbpl.mozilla.org/php/getParsedLog.php?id=11890401&tree=Mozilla-Inbound

11099 INFO TEST-START | /tests/image/test/mochitest/test_bug733553.html
++DOMWINDOW == 45 (0xc27fa98) [serial = 613] [outer = 0xac8d138]
++DOCSHELL 0xcdadd58 == 14 [id = 192]
++DOMWINDOW == 46 (0xd2c5638) [serial = 614] [outer = (nil)]
++DOMWINDOW == 47 (0xc813968) [serial = 615] [outer = 0xd2c55f0]
11100 INFO TEST-PASS | /tests/image/test/mochitest/test_bug733553.html | Test 0 red.png width not correct - 1 should equal 1
11101 INFO TEST-PASS | /tests/image/test/mochitest/test_bug733553.html | Test 1 animated-gif2.gif width not correct - 40 should equal 40
11102 INFO TEST-PASS | /tests/image/test/mochitest/test_bug733553.html | Test 2 red.png width not correct - 1 should equal 1
###!!! ABORT: Should have given mStatusTracker to mImage: '!mStatusTracker', file ../../../image/src/imgRequest.cpp, line 187
std::vector<nsRefPtr<imgCacheEntry>, std::allocator<nsRefPtr<imgCacheEntry> > >::push_back(nsRefPtr<imgCacheEntry> const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<nsRefPtr<imgCacheEntry>, std::allocator<nsRefPtr<imgCacheEntry> > >::push_back(nsRefPtr<imgCacheEntry> const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<unsigned short, std::allocator<unsigned short> >::resize(unsigned int, unsigned short) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<unsigned short, std::allocator<unsigned short> >::resize(unsigned int, unsigned short) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<unsigned short, std::allocator<unsigned short> >::resize(unsigned int, unsigned short) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
mozilla::scache::PathifyURI(nsIURI*, nsACString_internal&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<unsigned short, std::allocator<unsigned short> >::resize(unsigned int, unsigned short) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<unsigned short, std::allocator<unsigned short> >::resize(unsigned int, unsigned short) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<unsigned short, std::allocator<unsigned short> >::resize(unsigned int, unsigned short) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<unsigned short, std::allocator<unsigned short> >::resize(unsigned int, unsigned short) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
mozilla::services::_external_GetHistoryService() (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
XRE_AddStaticComponent (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<mozilla::plugins::IPCByteRange, std::allocator<mozilla::plugins::IPCByteRange> >::resize(unsigned int, mozilla::plugins::IPCByteRange) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
void std::vector<std::pair<int, int>, std::allocator<std::pair<int, int> > >::emplace_back<std::pair<int, int> >(std::pair<int, int>&&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<base::Histogram*, std::allocator<base::Histogram*> >::push_back(base::Histogram* const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
std::vector<base::Histogram*, std::allocator<base::Histogram*> >::push_back(base::Histogram* const&) (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
JSD_GetValueForObject (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
js::SecurityWrapper<js::Wrapper>::~SecurityWrapper() (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
XRE_InitCommandLine (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
XRE_InitCommandLine (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
XRE_main (/home/cltbld/talos-slave/test/build/firefox/libxul.so)
stat (/home/cltbld/talos-slave/test/build/firefox/firefox-bin)
__libc_start_main+0x000000E6  (/lib/libc.so.6)
###!!! ABORT: Should have given mStatusTracker to mImage: '!mStatusTracker', file ../../../image/src/imgRequest.cpp, line 187
TEST-UNEXPECTED-FAIL | /tests/image/test/mochitest/test_bug733553.html | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:02:23.706346
INFO | automation.py | Reading PID log: /tmp/tmpCq-2Ifpidlog
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux-debug/1337441633/firefox-15.0a1.en-US.linux-i686.crashreporter-symbols.zip
PROCESS-CRASH | /tests/image/test/mochitest/test_bug733553.html | application crashed (minidump found)
Crash dump filename: /tmp/tmp-cGEmZ/minidumps/308c08d3-3592-9197-450db487-44bfa318.dmp
Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 0 (crashed)
 0  libmozalloc.so!TouchBadMemory [mozalloc_abort.cpp : 68 + 0x0]
    eip = 0x00e99efe   esp = 0xbf972148   ebp = 0xbf972168   ebx = 0x00e9b188
    esi = 0x00c59844   edi = 0xbf972198   eax = 0x0000000a   ecx = 0x00e9b188
    edx = 0x00c5a32c   efl = 0x00210206
    Found by: given as instruction pointer in context
 1  libmozalloc.so!mozalloc_abort [mozalloc_abort.cpp : 89 + 0x4]
    eip = 0x00e99f3f   esp = 0xbf972150   ebp = 0xbf972168   ebx = 0x00e9b188
    esi = 0x00c59844   edi = 0xbf972198
    Found by: call frame info
 2  libxul.so!NS_DebugBreak_P [nsDebugImpl.cpp : 417 + 0x5]
    eip = 0x02027705   esp = 0xbf972170   ebp = 0xbf9725a8   ebx = 0x02c10fc4
    esi = 0xbf972584   edi = 0xbf972198
    Found by: call frame info
 3  libxul.so!imgRequest::GetStatusTracker [imgRequest.cpp : 186 + 0x23]
    eip = 0x012aa43f   esp = 0xbf9725b0   ebp = 0xbf9725d8   ebx = 0x02c10fc4
    esi = 0x0bff0aa8   edi = 0xbf972698
    Found by: call frame info
 4  libxul.so!imgRequest::OnStartRequest [imgRequest.cpp : 802 + 0x8]
    eip = 0x012abe40   esp = 0xbf9725e0   ebp = 0xbf9726b8   ebx = 0x02c10fc4
    esi = 0x0bff0aa8   edi = 0xbf972698
    Found by: call frame info
 5  libxul.so!nsPartChannel::SendOnStartRequest [nsMultiMixedConv.cpp : 104 + 0x15]
    eip = 0x0118f42a   esp = 0xbf9726c0   ebp = 0xbf9726d8   ebx = 0x02c10fc4
    esi = 0x0df17360   edi = 0x0df17360
    Found by: call frame info
 6  libxul.so!nsMultiMixedConv::SendStart [nsMultiMixedConv.cpp : 868 + 0xe]
    eip = 0x01190844   esp = 0xbf9726e0   ebp = 0xbf972758   ebx = 0x02c10fc4
    esi = 0x00000000   edi = 0x0df17360
    Found by: call frame info
 7  libxul.so!nsMultiMixedConv::OnDataAvailable [nsMultiMixedConv.cpp : 577 + 0xa]
    eip = 0x01190da2   esp = 0xbf972760   ebp = 0xbf9727d8   ebx = 0x02c10fc4
    esi = 0x0d9e0374   edi = 0xbf972840
    Found by: call frame info
 8  libxul.so!ProxyListener::OnDataAvailable [imgLoader.cpp : 2102 + 0x22]
    eip = 0x012a3b4f   esp = 0xbf9727e0   ebp = 0xbf972808   ebx = 0x02c10fc4
    esi = 0x0d584a7c   edi = 0xbf972840
    Found by: call frame info
 9  libxul.so!nsStreamListenerTee::OnDataAvailable [nsStreamListenerTee.cpp : 122 + 0x22]
    eip = 0x0116d3d8   esp = 0xbf972810   ebp = 0xbf972878   ebx = 0x02c10fc4

Comment 46

[Adam]

Attachment: Fix broken assert situations.

Sorry for the trouble Ryan, but thanks for helping.  

The assert the test hit is fixed by also checking for |mGotData| in |imgRequest::GetStatusTracker|.  The issue there is that we can't remove the old mImage in |imgRequest::OnStartRequest|, but we do have to go ahead and create a new status tracker, so later in |OnStartRequest| it would hit that assert.  The guard of |mGotData| will ensure that the existing image branch is only used if that image is still in use.

Another one that wasn't caught (occurred later) was in |imgRequestProxy::SetImage|.  We're reusing the proxy so we have to update its image; here the assert is changed to alternatively pass if the owning request is multipart.

Comment 47

[Adam]

Attachment: Update test to give a little more time between sending parts.

I noticed an intermittent problem caused by short time between parts, so I've lengthened the time between the parts being sent.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=3799d5361148

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed488f577c84
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1c93b5b549

Comment 49

[Ryan VanderMeulen [:RyanVM]]

Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/b73ef4b11140
https://hg.mozilla.org/mozilla-central/rev/ca8a314452a2

We'll pick up the re-landing with the next inbound-->m-c merge.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #49)
> Original landing and backout:
> https://hg.mozilla.org/mozilla-central/rev/b73ef4b11140
> https://hg.mozilla.org/mozilla-central/rev/ca8a314452a2
> 
> We'll pick up the re-landing with the next inbound-->m-c merge.

Whoops, forgot to include the backout changeset.
https://hg.mozilla.org/mozilla-central/rev/9691915b4c0d

Comment 51

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #48)
> Green on Try.
> https://tbpl.mozilla.org/?tree=Try&rev=3799d5361148
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ed488f577c84
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1c93b5b549

https://hg.mozilla.org/mozilla-central/rev/ed488f577c84
https://hg.mozilla.org/mozilla-central/rev/8f1c93b5b549