Bug 187360

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=199580

Attachments:

Description	Flags
Patch	none
Patch	ggaren: review+

Alex Christensen

Reported 2018-07-05 13:05:33 PDT

Fix iOS crash when starting loads with no active DocumentLoader

Attachments
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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alex Christensen

Comment 1 2018-07-05 13:09:06 PDT

Created attachment 344355 [details] Patch

Daniel Bates

Comment 2 2018-07-05 14:07:02 PDT

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

Comment 3 2018-07-05 14:08:23 PDT

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

Daniel Bates

Comment 4 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.

Daniel Bates

Comment 5 2018-07-05 14:17:11 PDT

<rdar://problem/29389084>

Geoffrey Garen

Comment 6 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?

Alex Christensen

Comment 7 2019-06-17 16:58:00 PDT

Created attachment 372294 [details] Patch

Geoffrey Garen

Comment 8 2019-06-17 17:09:34 PDT

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

Comment 9 2019-06-17 17:15:12 PDT

http://trac.webkit.org/r246525

Note You need to log in before you can comment on or make changes to this bug.