Bug 129349 - Assertion failure at CachedResource.h:196: ASSERT(!m_purgeableData)
Summary: Assertion failure at CachedResource.h:196: ASSERT(!m_purgeableData)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-25 16:59 PST by Pratik Solanki
Modified: 2014-02-27 11:06 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2014-02-25 17:02 PST, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (1.79 KB, patch)
2014-02-25 17:03 PST, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (2.06 KB, patch)
2014-02-26 12:49 PST, Pratik Solanki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2014-02-25 16:59:08 PST
This assertion triggers frequently when launching Safari/loading URLs.
Comment 1 Pratik Solanki 2014-02-25 17:02:02 PST
Created attachment 225199 [details]
Patch
Comment 2 Pratik Solanki 2014-02-25 17:02:39 PST
<rdar://problem/14871837>
Comment 3 Pratik Solanki 2014-02-25 17:03:21 PST
Created attachment 225200 [details]
Patch
Comment 4 Darin Adler 2014-02-26 10:01:22 PST
Comment on attachment 225200 [details]
Patch

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

> Source/WebKit/mac/ChangeLog:11
> +        The code for clearing out memory mapped notification callbacks is only needed when loading
> +        PDFs. And in such cases, we always have dataSourceDelegate object. So make this code
> +        conditional on its presence so that we don't trigger the assert for non-PDF main resources.

Maybe the change is OK, but the code change itself is completely mysterious. There’s a null check of dataSourceDelegate, but nothing makes clear why it’s OK to skip the code below if the delegate is null. I think you need some kind of comment.
Comment 5 Pratik Solanki 2014-02-26 11:02:32 PST
(In reply to comment #4)
> (From update of attachment 225200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225200&action=review
> 
> > Source/WebKit/mac/ChangeLog:11
> > +        The code for clearing out memory mapped notification callbacks is only needed when loading
> > +        PDFs. And in such cases, we always have dataSourceDelegate object. So make this code
> > +        conditional on its presence so that we don't trigger the assert for non-PDF main resources.
> 
> Maybe the change is OK, but the code change itself is completely mysterious. There’s a null check of dataSourceDelegate, but nothing makes clear why it’s OK to skip the code below if the delegate is null. I think you need some kind of comment.

Yeah, I agree. I'll add comments in the change. I think Joe and I are leaning towards removing this whole disk image cache code path now that we have CFNetwork disk cache.
Comment 6 Pratik Solanki 2014-02-26 11:02:32 PST
(In reply to comment #4)
> (From update of attachment 225200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225200&action=review
> 
> > Source/WebKit/mac/ChangeLog:11
> > +        The code for clearing out memory mapped notification callbacks is only needed when loading
> > +        PDFs. And in such cases, we always have dataSourceDelegate object. So make this code
> > +        conditional on its presence so that we don't trigger the assert for non-PDF main resources.
> 
> Maybe the change is OK, but the code change itself is completely mysterious. There’s a null check of dataSourceDelegate, but nothing makes clear why it’s OK to skip the code below if the delegate is null. I think you need some kind of comment.

Yeah, I agree. I'll add comments in the change. I think Joe and I are leaning towards removing this whole disk image cache code path now that we have CFNetwork disk cache.
Comment 7 Pratik Solanki 2014-02-26 12:49:38 PST
Created attachment 225284 [details]
Patch
Comment 8 Joseph Pecoraro 2014-02-27 10:44:31 PST
Comment on attachment 225284 [details]
Patch

r=me, This is a harmless addition to avoid an assert we know about and can't easily fix. We want to just remove DiskImageCache altogether eventually.
Comment 9 WebKit Commit Bot 2014-02-27 11:06:21 PST
Comment on attachment 225284 [details]
Patch

Clearing flags on attachment: 225284

Committed r164817: <http://trac.webkit.org/changeset/164817>
Comment 10 WebKit Commit Bot 2014-02-27 11:06:24 PST
All reviewed patches have been landed.  Closing bug.