Bug 23180 – Reading freed memory at DocumentLoader::checkForPendingPreloads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mads Ager]

In loader.cpp in Loader::Host::didFinishLoading the following two lines causes the problem:

 docLoader->setLoadInProgress(false);
 docLoader->checkForPendingPreloads();

setLoadInProgress(false) can cause the frame to be deallocated, which can cause the document to be deallocated and the document destructor deletes the DocumentLoader.  Therefore, docLoader can be freed memory at the call to checkForPendingPreloads.

This happens when running the layout test: LayoutTests/http/tests/misc/onload-remove-iframe-crash-2.html.

Would it make sense to just reorder and check for pending preloads before setting load in progress to false?

[Antti Koivisto]

Sounds sensible

[Jon@Chromium]

See http://code.google.com/p/chromium/issues/detail?id=3949

[Marc-André Decoste]

Another solution I tried while working on the Chromium version of this bug is to hold on to a reference of the document while we interact with the docLoader... This would make the code safer for future maintenance and would not depend on the call order...

I have a patch with that change (which is needed at two places by the way), that I will send for code review soon (once I know how to do this here, first time I try :-).


(In reply to comment #0)
> In loader.cpp in Loader::Host::didFinishLoading the following two lines causes
> the problem:
> 
>  docLoader->setLoadInProgress(false);
>  docLoader->checkForPendingPreloads();
> 
> setLoadInProgress(false) can cause the frame to be deallocated, which can cause
> the document to be deallocated and the document destructor deletes the
> DocumentLoader.  Therefore, docLoader can be freed memory at the call to
> checkForPendingPreloads.
> 
> This happens when running the layout test:
> LayoutTests/http/tests/misc/onload-remove-iframe-crash-2.html.
> 
> Would it make sense to just reorder and check for pending preloads before
> setting load in progress to false?
> 

[Marc-André Decoste]

Created attachment 27203 [details]
Patch

To fix the bug as proposed in bug comments from me (mad).

[Antti Koivisto]

Looks good, two comments:
- I believe "protector" is the usual name for this kind of variables in WebKit. I don't know what "gard" means.
- Please provide a ChangeLog entry.

[Marc-André Decoste]

Created attachment 27252 [details]
New version of the patch based on recent comments.

Changed the variable name from gard (term used in Chromium, this is why I used it ;-), to protector which is more widely used in WebKit.

Also added details to ChangeLog file (slowly getting used to the process :-)

And fixed comments to be grammatically correct (start with a Capital and end with a period.). :-)

[Antti Koivisto]

Comment on attachment 27252 [details]
New version of the patch based on recent comments.

r=me

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/loader/loader.cpp
Committed r40648