Aryeh Gregor (:ayg) (no longer with Mozilla) Reporter
	Description • 12 years ago

http://dxr.lanedo.com/mozilla-central/image/decoders/nsJPEGDecoder.cpp.html?string=nsJPEGDecoder.cpp#l190

  /* Return here if there is a fatal error within libjpeg. */
  nsresult error_code;
  if ((error_code = setjmp(mErr.setjmp_buffer)) != 0) {
    if (error_code == NS_ERROR_FAILURE) {
      PostDataError();
      /* Error due to corrupt stream - return NS_OK and consume silently
         so that libpr0n doesn't throw away a partial image load */
      mState = JPEG_SINK_NON_JPEG_TRAILER;
      PR_LOG(gJPEGDecoderAccountingLog, PR_LOG_DEBUG,
             ("} (setjmp returned NS_ERROR_FAILURE)"));
      return;
    } else {
      /* Error due to reasons external to the stream (probably out of
         memory) - let libpr0n attempt to clean up, even though
         mozilla is seconds away from falling flat on its face. */
      PostDecoderError(error_code);
      mState = JPEG_ERROR;
      PR_LOG(gJPEGDecoderAccountingLog, PR_LOG_DEBUG,
             ("} (setjmp returned an error)"));
      return;
    }
  }

This is, to put it mildly, unlikely to produce expected results.  The NS_ERROR_FAILURE path is presumably impossible.  The other path will call PostDecoderError, which will either call NS_ABORT_IF_FALSE if the high bit of error_code happens to be 0, or will store it and report gibberish.

I don't understand this code does well enough to actually fix it, so I'm just reporting it.  In bug 777292 I wrote a patch that adds a cast to nsresult so that it will actually compile once nsresult type-safety is enforced, plus a comment pointing out that the code is wrong.

	Aryeh Gregor (:ayg) (no longer with Mozilla) Reporter
	Comment 1 • 12 years ago

Actually, I just didn't understand what setjmp()/longjmp() do.  setjmp() returns whatever longjmp() was passed, which in this case is actually NS_ERROR_FAILURE.  This mixing of nsresult and int is still confusing, however, and IMO it would be better to use some other integer value like -1 so that we don't have to cast things.  See also another patch related to this in bug 782252.