Bug 187360 - Fix iOS crash when starting loads with no active DocumentLoader
Summary: Fix iOS crash when starting loads with no active DocumentLoader
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
Keywords: InRadar
Depends on:
Reported: 2018-07-05 13:05 PDT by Alex Christensen
Modified: 2019-07-08 11:19 PDT (History)
8 users (show)

See Also:

Patch (1.55 KB, patch)
2018-07-05 13:09 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (1.93 KB, patch)
2019-06-17 16:58 PDT, Alex Christensen
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2018-07-05 13:05:33 PDT
Fix iOS crash when starting loads with no active DocumentLoader
Comment 1 Alex Christensen 2018-07-05 13:09:06 PDT
Created attachment 344355 [details]
Comment 2 Daniel Bates 2018-07-05 14:07:02 PDT
Comment on attachment 344355 [details]

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.
Comment 3 Alex Christensen 2018-07-05 14:08:23 PDT
I can't reproduce this.  I'm only reading logs.
Comment 4 Daniel Bates 2018-07-05 14:16:14 PDT
(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.
Comment 5 Daniel Bates 2018-07-05 14:17:11 PDT
Comment 6 Geoffrey Garen 2019-06-17 16:37:19 PDT
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?
Comment 7 Alex Christensen 2019-06-17 16:58:00 PDT
Created attachment 372294 [details]
Comment 8 Geoffrey Garen 2019-06-17 17:09:34 PDT
Comment on attachment 372294 [details]


Side note: should we really call cancel() here? cancel() seems to dereference m_documentLoader, so we might just move the crash point.
Comment 9 Alex Christensen 2019-06-17 17:15:12 PDT