Bug 33827 – Assertion failure in DOMApplicationCache constructor

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

[Alexey Proskuryakov]

DOMApplicationCache::DOMApplicationCache(Frame* frame)
    : m_frame(frame)
{
    ASSERT(!m_frame || applicationCacheHost());

This assertion is clearly incorrect, because applicationCacheHost() returns null if m_frame is null. So, it effectively asserts that applicationCacheHost() is not null. This also isn't the case in attached test case.

[Alexey Proskuryakov]

Created attachment 46886 [details]
test case (will assert)

[Alexey Proskuryakov]

I don't really understand what ApplicationCacheHost is, so it's not quite clear to me whether it's ok to just remove this assertion, or whether null host should prevent DOMApplicationCache construction.

[Michael Nordman]

I'll take a look.

[Michael Nordman]

The intent of the assertion is to gaurantee that if there is a frame, there must be an applicationCacheHost. This is recognizing 'frameless' construction is a valid thing to do (i don't know what the use case is for that, but Alexey insisted that these should be constructable w/o a frame at some point).

[Michael Nordman]

DOMApplicationCache.m_frame == NULL when i run the test case in chrome (and chrome's test_shell). So no assertions. There may be recent changes that haven't rolled into view yet for chrome?

[Alexey Proskuryakov]

> Alexey insisted that these should be constructable w/o a frame at some point

I could have been wrong! I still think that this would be a reasonable behavior, but there's a lot of code in WebCore that doesn't work in frameless documents.

Do other constructors work in detached frames? What about window.history, or window.statusbar?

> DOMApplicationCache.m_frame == NULL when i run the test case in chrome  

Re-checked with r53418, still getting a non-null detached frame here.

[Michael Nordman]

All detached frames or just about:blanks?

What is the expected behavior in a 'detached' frame anyway... are timers and xhrs and subresource loads expected to function properly?

[Michael Nordman]

After chatting with alexey, decided to remove this assertion. Its there to test my assumption that if we have a frame, it must have a documentLoader, which is not always the case (apparently).

I'll put together a patch soon'ish.

Also realized that we have to look at detached frame handling a little more closely throughout the scriptable objects dangling off of window properties.

[Michael Nordman]

Created attachment 47065 [details]
removeAssert

[Alexey Proskuryakov]

Comment on attachment 47065 [details]
removeAssert

>          No new functionality, no new tests.

r=me, assuming you'll land the test simultaneously (be sure to make it dumpAsText).

[Michael Nordman]

Created attachment 47079 [details]
removeAssert2

Now with a layout test to verify that the assertion has been removed.

[Alexey Proskuryakov]

Comment on attachment 47079 [details]
removeAssert2

r=me

This test doesn't need waitUntilDone/notifyDone - it runs from onload, and doesn't use setTimeout.

[Michael Nordman]

Created attachment 47082 [details]
removeAssert3

Now without waitUntilDone/notifyDone, and with closing tags and a blank line.

[WebKit Commit Bot]

Comment on attachment 47082 [details]
removeAssert3

Clearing flags on attachment: 47082

Committed r53596: <http://trac.webkit.org/changeset/53596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.