Closed Bug 858818 Opened 10 years ago Closed 10 years ago

Assertion failure: !constructed

Categories

(Core :: Graphics: ImageLib, defect)

22 Branch
defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 863958

People

(Reporter: bc, Assigned: seth)

References

()

Details

(Keywords: assertion)

Attachments

(3 files)

Attached file crash report
1. http://sweetp-paulette.blogspot.ca/
2. Wait for it.
    Assertion failure: !constructed, at ../../dist/include/mozilla/Util.h:173

void mozilla::Maybe<nsIntSize>::construct<nsIntSize> mozilla::image::ImageMetadata::SetSize mozilla::image::Decoder::PostSize mozilla::image::nsICODecoder::WriteInternal mozilla::image::Decoder::Write

Aurora/22, Nightly/23 Windows, Linux, OSX
At a first glance, it looks like ImageMetadata::SetSize should allow SetSize() to be called more than once, especially given the name of the method.  Maybe there is another reason why we don't want that?
Blocks: 716140
Comment on attachment 734753 [details] [diff] [review]
If SetSize() can be called multiple times, we should be ready for it.

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

Let's hold onto the few invariants we have, Milan! Clutch them for all that they're worth!
Attachment #734753 - Flags: feedback?(seth) → feedback-
Here's an alternate approach that maintains the "only call PostSize once" invariant. We only call PostSize if the embedded PNG decoder found the size during this particular call to WriteInternal.

Milan, can you review this? I speculatively assigned it to you.
Attachment #734888 - Flags: review?(milan)
Assignee: nobody → seth
We need a test for this. Should be fairly easy to reproduce. This will happen any time we have an .ICO with an embedded .PNG and the .PNG is large enough that nsICODecoder::WriteInternal can't finish the whole thing in one go-round.
Bummer. So far I haven't had any luck producing a working test, because (as is obvious in retrospect) this also needs a slow network connection, so we don't get in the entire icon in one big chunk. I'll leave a test for this aside for now, because I don't know how to simulate a slow network connection in this way, and nobody on IRC seemed to know either. (Though it seems a useful feature, and I wish we had it baked in to our testing tools.)
Seth, are you on OSX?
(In reply to Seth Fowler [:seth] from comment #6)
> Bummer. So far I haven't had any luck producing a working test, because (as
> is obvious in retrospect) this also needs a slow network connection, 

This fails for me in debug every time, specifying the URL on the command line.  On OSX.
Comment on attachment 734888 [details] [diff] [review]
nsICODecoder should PostSize only once.

As a nit, I would rename bool mContainedDecoderHasSize, so that it doesn't look like a member variable.

There is another call to PostSize further in the file, and that one isn't protected.  Could that get hit for BMP icons, given that it seems that WriterInternal() can get called more than once?

Overall, it appears a bit fragile, in that PostSize() should only get called once in the derived classes, but that isn't obvious just looking at the code, as witnessed by the example where it fails to do that :)
(In reply to Bob Clary [:bc:] from comment #7)
> Seth, are you on OSX?

I am. Why do you ask? I'm trying to write a standalone test, not reproduce the problem with the original site, which I can definitely do.
Sorry, was just going to recommend ipfw but that won't help for a general standalone test.
(In reply to Bob Clary [:bc:] from comment #11)
> Sorry, was just going to recommend ipfw but that won't help for a general
> standalone test.

No worries. Given that both you and Milan seem to have misunderstood me I should have phrased things a little more clearly. =)
Comment on attachment 734888 [details] [diff] [review]
nsICODecoder should PostSize only once.

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

There is a PostSize() for GIF files as well - should that be handled as well?

::: image/decoders/nsICODecoder.cpp
@@ +344,5 @@
>    }
>  
>    // If we have a PNG, let the PNG decoder do all of the rest of the work
>    if (mIsPNG && mContainedDecoder && mPos >= mImageOffset + PNGSIGNATURESIZE) {
> +    bool mContainedDecoderHadSize = mContainedDecoder->HasSize();

Nit: rename mContainedDecoderHadSize, as it looks like a member variable.
Attachment #734888 - Flags: review?(milan) → review+
Er, sorry guys, I have a totally different implementation in bug 863958, which I think is probably better still.
(and it includes a testcase! :)
Yes, that's much better. Thanks Joe.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.