RasterImage::mInDecoder is now obsolete (I think)




5 years ago
3 years ago


(Reporter: Joe Drew (not getting mail), Unassigned)


Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
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.
I will look into this soon.

Thanks for assigning it to me :)
Created attachment 731431 [details] [diff] [review]
removed mInDecoder

This is the patch right now, still studying the implications of removing the use of this mInDecoder lock.
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.
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 :)
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 :)
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.
(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.)
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.
(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.
Great :)

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


5 years ago
Depends on: 898569
Seth removed this in Bug 1089046.
Assignee: luis → nobody
Last Resolved: 3 years ago
Resolution: --- → FIXED
No longer blocks: 1089046
Depends on: 1089046
You need to log in before you can comment on or make changes to this bug.