Bug 129349 – Assertion failure at CachedResource.h:196: ASSERT(!m_purgeableData)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Pratik Solanki]

This assertion triggers frequently when launching Safari/loading URLs.

[Pratik Solanki]

Created attachment 225199 [details]
Patch

[Pratik Solanki]

<rdar://problem/14871837>

[Pratik Solanki]

Created attachment 225200 [details]
Patch

[Darin Adler]

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.

[Pratik Solanki]

(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.

[Pratik Solanki]

(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.

[Pratik Solanki]

Created attachment 225284 [details]
Patch

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

Comment on attachment 225284 [details]
Patch

Clearing flags on attachment: 225284

Committed r164817: <http://trac.webkit.org/changeset/164817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.