Bug 23006 – Many Loader::Host member functions are not safe to use reentrantly

• Component: Page Loading
• Status: RESOLVED FIXED

[Cameron Zwarich (cpst)]

Many Loader::Host member functions set m_processingResource to true when they begin processing a resource and set it to false when they are done. Thanks to JavaScript and the web inspector, almost anything can happen during the processing of a resource, including these functions being called reentrantly, which is unsafe due to this way of using m_processingResource. Since m_processingResource is used to determine whether a Host is deleted in Loader::servePendingRequests(), this is presumedly the cause behind memory corruption bugs like <rdar://problem/6216106>.

[Cameron Zwarich (cpst)]

<rdar://problem/6216106>

[Cameron Zwarich (cpst)]

Created attachment 26275 [details]
Proposed patch

I am looking for a lot of review comments on this one. If anything comes to mind, don't assume that I thought about it and did it right. ;-)

[Darin Adler]

Comment on attachment 26275 [details]
Proposed patch

This patch is fine. We could also instead have ProcessingResource save and restore the m_processingResource boolean, which would use more stack space but less space in the object. It's really just fine as is. I'm going to say r=me

I don't understand your comment "There are no occurrences of crashes caused by this bug that are reproducible by multiple people". Who ran into this? How did you diagnose it?

[Cameron Zwarich (cpst)]

I diagnosed the problem in the Radar because people were saying that it happens a lot when they are using the debugger in the inspector, which I verified causes this bad reentrancy after breaking in GDB and noticing that the JS debugger reenters the event loop. I couldn't get it to actually delete one of the Hosts that it flagged as being safe for deletion. Other people had test cases they claimed were reproducible for them at various times, but I could never make any of them work for me. Dan Bernstein was in GDB for a particular occurrence of the bug, and said that it seemed that control was being returned to Loader::Host::didFinishLoading() with a freed Loader::Host(). I just guessed that this might be the problem from all the information given.

[Darin Adler]

OK.

[Cameron Zwarich (cpst)]

Landed in r39494.