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

NEW
Unassigned

Status

()

Core
ImageLib
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, memory-leak, testcase})

Trunk
x86_64
Mac OS X
assertion, memory-leak, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 716374 [details]
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.
(Reporter)

Comment 1

5 years ago
Created attachment 716375 [details]
stack
I can't seem to reproduce.... :(  I never see a white page.
(Reporter)

Comment 3

5 years ago
Try with different timeout values in place of 65?

Or maybe we can try debugging this over IRC?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)

Comment 5

5 years ago
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.
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.
Flags: needinfo?(bugs)
Blocks: 846759

Updated

5 years ago
Flags: needinfo?(bugs) → needinfo?(joe)
ImageDocument::Destroy unregisters with nsImageLoadingContent, so I think this is a leak, though to be sure we should really run with valgrind or ASAN.
Flags: needinfo?(joe)
Destroy is only called on documents in ... some cases, no?
I am out of my depth when it comes to the documents, unfortunately.
Flags: needinfo?(khuey)
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.
Flags: needinfo?(khuey)
How bad is this assertion, securitywise?
(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).
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.
Group: core-security
Keywords: mlk
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.
No longer blocks: 846759
You need to log in before you can comment on or make changes to this bug.