843474 - "ASSERTION: Observers still registered?" with image document, navigation

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Jesse Ruderman]

Attachment: testcase (see comment 0)

The testcase has fragile timing, but I can reproduce with a debug or debug-opt build.

1. Enable popups:
     user_pref("dom.disable_open_during_load", false);
2. Load the testcase by giving it to firefox as a command-line argument.
3. Make sure you see a data:image/gif,... URL in the address bar and a white page.  If not, the timing was off.
4. Wait for the next CC(?) or shut down.

Result:

###!!! ASSERTION: Observers still registered?: '!mObserverList.mObserver && !mObserverList.mNext', file content/base/src/nsImageLoadingContent.cpp, line 109

This assertion message also appears in bug 842903, fwiw.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

I can't seem to reproduce.... :(  I never see a white page.

Comment 3

[Jesse Ruderman]

Try with different timeout values in place of 65?

Or maybe we can try debugging this over IRC?

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so I think I managed to trigger this.

The observer is an ImageDocument.  

The destructor is being called via ContentUnbinder::Run.

Looks like this is something done via unlink.  Maybe we should drop the mObserver weak ref during unlink too?  Alternately, unlink on the ImageDocument should make sure to drop it.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

If the problem can happen because of ContentUnbinder, it can most probably happen also
in some other ways. (we don't always end up using ContentUnbinder).

Sounds like ImageDocument should make sure to drop it.

Comment 6

[Daniel Veditz [:dveditz]]

Is this a potential use-after-free (the observer isn't really there) or a memory leak? The latter wouldn't be a security problem.

Comment 7

[Joe Drew (not getting mail)]

ImageDocument::Destroy unregisters with nsImageLoadingContent, so I think this is a leak, though to be sure we should really run with valgrind or ASAN.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Destroy is only called on documents in ... some cases, no?

Comment 9

[Joe Drew (not getting mail)]

I am out of my depth when it comes to the documents, unfortunately.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I have no idea.  Should be easy enough to check if you're looking at this in a debugger though.  See if mObservingImageLoader is still true.

Comment 11

[Andrew McCreight [:mccr8]]

How bad is this assertion, securitywise?

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #7)
> ImageDocument::Destroy unregisters with nsImageLoadingContent, so I think
> this is a leak, though to be sure we should really run with valgrind or ASAN.

No extra complaints with the ASAN build (I do see the assertion though).

Comment 13

[Daniel Veditz [:dveditz]]

ASan doesn't complain about leaks (which aren't usually security issues) but will complain about use-after-frees. I guess this is OK. Valgrind can be told to check for leaks if someone wants to confirm it, but I'm happy enough to assume at this point.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

For what it's worth:

When things work (good timing)
a) we create ImageDocument
b) attach observer in OnStartRequest
c) attach and remove observers from nsImageFrame (twice)
d) call ImageDocument::Destroy which takes cares of the observer in step b)
e) we create a second ImageDocument (the first one's destructor hasn't fired yet)
f) do other observer related stuff

When things fail (bad timing):
1) we create ImageDocument
2) attach observer in OnStartRequest
3) attach and remove observers from nsImageFrame (twice)
4) we try to delete the nsImageLoadingContent all hit the assertion
5) on quit, we try to remove the observer from step 2) and can't find it (of course)

In the working case, we have two ImageDocuments, each with nsImageLoadingContent.  In the failing case, we have a single ImageDocument, with two different nsImageLoadingContents.