Bug 187360 – Fix iOS crash when starting loads with no active DocumentLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Fix iOS crash when starting loads with no active DocumentLoader

[Alex Christensen]

Created attachment 344355 [details]
Patch

[Daniel Bates]

Comment on attachment 344355 [details]
Patch

Can you please give an example (even better a test case since this is crash) of when m_documentLoader is null when this function is called? ResourceLoader assumes we always have a non-null pointer to a document loader unless it explicitly nullified it in ResourceLoader::releaseResources(). I mean, even in this function, we assume a non-null m_documentLoader when we ASSERT(!m_documentLoader->isSubstituteLoadPending(this)) at the top of this function.

[Alex Christensen]

I can't reproduce this.  I'm only reading logs.

[Daniel Bates]

(In reply to Alex Christensen from comment #3)
> I can't reproduce this.  I'm only reading logs.

I take it is not straightforward to read the code and look at the callers and/or use a debugger to try to piece together a scenario that explains the crash?

Are you able to post the crash report? Maybe we can leverage more minds to help identify the cause.

[Daniel Bates]

<rdar://problem/29389084>

[Geoffrey Garen]

We do generally accept crash fixes for top crashes based on field data, even if we only have a limited understanding of the data. So, I'm inclined to say r+.

That said, I do agree that it's surprising, and probably bad style, to null-check m_documentLoader after having used m_documentLoader.

Alex, could you put the null check at the top of the function, with an explicit comment linking to a bug report, explaining the m_documentLoader is not expected to be null, but is observed to be in some crash reports?

[Alex Christensen]

Created attachment 372294 [details]
Patch

[Geoffrey Garen]

Comment on attachment 372294 [details]
Patch

r=me

Side note: should we really call cancel() here? cancel() seems to dereference m_documentLoader, so we might just move the crash point.

[Alex Christensen]

http://trac.webkit.org/r246525