| Summary: | Fix iOS crash when starting loads with no active DocumentLoader | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aestes, beidson, cdumez, dbates, ews-watchlist, ggaren, japhet, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=199580 | ||||||||
| Attachments: |
|
||||||||
|
Description
Alex Christensen
2018-07-05 13:05:33 PDT
Created attachment 344355 [details]
Patch
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.
I can't reproduce this. I'm only reading logs. (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. 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? Created attachment 372294 [details]
Patch
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.
|