854506 - RasterImage::mInDecoder is now obsolete (I think)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

I am 98% sure that the field RasterImage::mInDecoder is now obsolete. Its purpose was to make it impossible to have reentrantcy into decoders, but that is mostly impossible now that we notify from SyncNotifyDiff instead of during the decode.

However, to prove that it's correct, someone needs to audit the imgDecoderObserver interface and all its users to make sure that it's not possible to hit any |if (mInDecoder)| blocks inside RasterImage. Of special note is that RasterImage::OnImageDataComplete can very easily result in, for example, RasterImage::StartDecoding being called, since it calls RasterImage::FinishedSomeDecoding, which *does* notify.

Comment 1

[Luis de Bethencourt [:luisbg]]

I will look into this soon.

Thanks for assigning it to me :)

Comment 2

[Luis de Bethencourt [:luisbg]]

Attachment: removed mInDecoder

This is the patch right now, still studying the implications of removing the use of this mInDecoder lock.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Luis, please also consider (depending on the size of the change that would be required) restructuring the code to make it easier to prove that we can't reenter decoders. IMO if we can't answer this question easily, that is a bug in itself.

Comment 4

[Luis de Bethencourt [:luisbg]]

The above patch does NOT work. But it is valuable first step towards investigating this issue.

Seth, will look into proving we can't reenter decoders. Thanks for the comment :)

Comment 5

[Luis de Bethencourt [:luisbg]]

Actually, the bug I was seeing was somewhere else and now fixed in latest hg head.

Can somebody explain me how to write/run automated tests to confirm this? I have so far tested this extensively but manually and would like to test it properly. Will be valuable to learn how to write automated tests for the future :)

Comment 6

[Seth Fowler [:seth] [:s2h]]

Perhaps fuzz testing and similar could be helpful but overall I don't think testing is the right approach here. Testing is not very good for establishing that something can never happen. You may find it useful if there are specific scenarios that you want to check, but in general you are going to have to establish that removing mInDecoder is safe by argument.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(This is why I suggest you may want to actually make code changes; if the structure of the code makes it too difficult to establish whether mInDecoder is needed, the best solution may well be to change the structure of the code.)

Comment 8

[Luis de Bethencourt [:luisbg]]

Thinking about this yes, I agree with you that automated testing isn't really that helpful.

I will look into the possibility of re-entering decoders, proving that this can't happen, and restructuring the code. I understand the initial design of mInDecoder and why it isn't needed anymore. But to prove it completely I need to understand the code flow of RasterImages.

Do you have any recommendations of how to learn about the code flow/logic? Is there some resource, document or sequenced test to read. Or just adding break points/outputs in the code and reading it thoroughly?

Thanks for all the help Seth.

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Luis de Bethencourt [:luisbg] from comment #8)
> Is there some resource, document or sequenced test to read.

There is very little useful formal documentation; you'll mostly just have to read and get in there with a debugger, as you say. One additional thing that I have sometimes found very helpful is hg blame. You can use this to find out which commit (and hence which bug) introduced a certain line of the code. Reading the Bugzilla page for that bug is often the best documentation available.

Comment 10

[Luis de Bethencourt [:luisbg]]

Great :)

Thanks a lot for the pointers Seth. Will keep you up to date with my findings.

Comment 11

[Robert Lickenbrock [:rclick]]

Seth removed this in Bug 1089046.