Bug 105591 - REGRESSION (r138222?): Assertion failure on appcache/main-resource-redirect.html
Summary: REGRESSION (r138222?): Assertion failure on appcache/main-resource-redirect.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords: Regression
: 105691 105788 (view as bug list)
Depends on:
Blocks: 106123
  Show dependency treegraph
 
Reported: 2012-12-20 17:17 PST by Alexey Proskuryakov
Modified: 2013-01-04 12:10 PST (History)
7 users (show)

See Also:


Attachments
patch (3.74 KB, patch)
2012-12-21 11:03 PST, Nate Chapin
beidson: review+
japhet: commit-queue?
Details | Formatted Diff | Diff
Different approach (3.96 KB, patch)
2012-12-28 01:00 PST, Carlos Garcia Campos
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-12-20 17:17:12 PST
Looks like <http://trac.webkit.org/r138222> made it so that a delegate is called for a resource with 0 identifier on this test, so a crash happens when accessing a HashMap.

Nate, can you please take a look?

Note the test is currently also affected by an unrelated regression, tracked as bug 105563.
Comment 1 Brady Eidson 2012-12-20 17:24:54 PST
Resources need to always have an identifier.
Comment 2 Nate Chapin 2012-12-21 11:03:37 PST
Created attachment 180537 [details]
patch
Comment 3 Alexey Proskuryakov 2012-12-23 11:56:12 PST
*** Bug 105691 has been marked as a duplicate of this bug. ***
Comment 4 Alexey Proskuryakov 2012-12-27 12:08:15 PST
*** Bug 105788 has been marked as a duplicate of this bug. ***
Comment 5 Carlos Garcia Campos 2012-12-28 00:48:46 PST
Comment on attachment 180537 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180537&action=review

> Source/WebCore/loader/MainResourceLoader.cpp:594
> +    m_substituteDataLoadIdentifier = m_documentLoader->frame()->page()->progress()->createUniqueIdentifier();
> +    frameLoader()->notifier()->assignIdentifierToInitialRequest(m_substituteDataLoadIdentifier, documentLoader(), m_initialRequest);
> +    frameLoader()->notifier()->dispatchWillSendRequest(documentLoader(), m_substituteDataLoadIdentifier, m_initialRequest, ResourceResponse());

In case of redirection a new identifier will be created, and assignIdentifierToInitialRequest() will be called twice, this request is not the initial one, but the redirected one, shouldn't we consider it like the same resource and keep the identifier? See the patch I attached to bug #105788
Comment 6 Carlos Garcia Campos 2012-12-28 01:00:34 PST
Created attachment 180851 [details]
Different approach

In case of redirect, assignIdentifierToInitialRequest has already been called for the resource, if the redirect is going to be loaded from the app cache reuse the identifier and continue the load.
Comment 7 Brady Eidson 2013-01-03 10:14:46 PST
Comment on attachment 180851 [details]
Different approach

I prefer this one over Nate's, though both are acceptable if tests pass, etc etc...
Comment 8 Nate Chapin 2013-01-03 10:17:42 PST
Comment on attachment 180851 [details]
Different approach

View in context: https://bugs.webkit.org/attachment.cgi?id=180851&action=review

> Source/WebCore/loader/MainResourceLoader.cpp:273
> +        if (m_substituteData.isValid())
> +            m_substituteDataLoadIdentifier = identifier();

I don't know if this was tested in debug, but it's possible that this will assert when identifier() is called later, since it asserts that either m_substituteDataLoadIdentifer or ResourceLoader::identiifer() will be 0.  Please verify before landing if you haven't already done so.
Comment 9 Chris Dumez 2013-01-03 10:58:16 PST
Comment on attachment 180851 [details]
Different approach

View in context: https://bugs.webkit.org/attachment.cgi?id=180851&action=review

>> Source/WebCore/loader/MainResourceLoader.cpp:273
>> +            m_substituteDataLoadIdentifier = identifier();
> 
> I don't know if this was tested in debug, but it's possible that this will assert when identifier() is called later, since it asserts that either m_substituteDataLoadIdentifer or ResourceLoader::identiifer() will be 0.  Please verify before landing if you haven't already done so.

Yes, I verified that this patch fixed http/tests/appcache/main-resource-redirect.html for a debug build of WebKit2 EFL. I'll do a layout tests run though to make sure there are no regressions.
Comment 10 Chris Dumez 2013-01-03 12:34:09 PST
(In reply to comment #9)
> (From update of attachment 180851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180851&action=review
> 
> >> Source/WebCore/loader/MainResourceLoader.cpp:273
> >> +            m_substituteDataLoadIdentifier = identifier();
> > 
> > I don't know if this was tested in debug, but it's possible that this will assert when identifier() is called later, since it asserts that either m_substituteDataLoadIdentifer or ResourceLoader::identiifer() will be 0.  Please verify before landing if you haven't already done so.
> 
> Yes, I verified that this patch fixed http/tests/appcache/main-resource-redirect.html for a debug build of WebKit2 EFL. I'll do a layout tests run though to make sure there are no regressions.

I did a full layout tests run with a debug build. The patch does not seem to cause regressions on EFL WK2.
Comment 11 Carlos Garcia Campos 2013-01-04 01:05:18 PST
Committed r138782: <http://trac.webkit.org/changeset/138782>
Comment 12 Ryosuke Niwa 2013-01-04 12:10:14 PST
Apparently this crash hasn’t been fixed. See https://bugs.webkit.org/show_bug.cgi?id=106123.