Bug 33827 - Assertion failure in DOMApplicationCache constructor
Summary: Assertion failure in DOMApplicationCache constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-18 22:51 PST by Alexey Proskuryakov
Modified: 2010-01-20 20:07 PST (History)
3 users (show)

See Also:


Attachments
test case (will assert) (223 bytes, text/html)
2010-01-18 22:52 PST, Alexey Proskuryakov
no flags Details
removeAssert (1.19 KB, patch)
2010-01-20 15:04 PST, Michael Nordman
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
removeAssert2 (3.13 KB, patch)
2010-01-20 16:49 PST, Michael Nordman
ap: review+
Details | Formatted Diff | Diff
removeAssert3 (2.99 KB, patch)
2010-01-20 17:14 PST, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-01-18 22:51:27 PST
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.
Comment 1 Alexey Proskuryakov 2010-01-18 22:52:00 PST
Created attachment 46886 [details]
test case (will assert)
Comment 2 Alexey Proskuryakov 2010-01-20 12:10:29 PST
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.
Comment 3 Michael Nordman 2010-01-20 12:44:38 PST
I'll take a look.
Comment 4 Michael Nordman 2010-01-20 12:50:47 PST
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).
Comment 5 Michael Nordman 2010-01-20 13:19:23 PST
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?
Comment 6 Alexey Proskuryakov 2010-01-20 13:30:49 PST
> 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.
Comment 7 Michael Nordman 2010-01-20 13:36:18 PST
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?
Comment 8 Michael Nordman 2010-01-20 14:04:11 PST
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.
Comment 9 Michael Nordman 2010-01-20 15:04:54 PST
Created attachment 47065 [details]
removeAssert
Comment 10 Alexey Proskuryakov 2010-01-20 15:12:34 PST
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).
Comment 11 Michael Nordman 2010-01-20 16:49:35 PST
Created attachment 47079 [details]
removeAssert2

Now with a layout test to verify that the assertion has been removed.
Comment 12 Alexey Proskuryakov 2010-01-20 16:56:15 PST
Comment on attachment 47079 [details]
removeAssert2

r=me

This test doesn't need waitUntilDone/notifyDone - it runs from onload, and doesn't use setTimeout.
Comment 13 Michael Nordman 2010-01-20 17:14:13 PST
Created attachment 47082 [details]
removeAssert3

Now without waitUntilDone/notifyDone, and with closing tags and a blank line.
Comment 14 WebKit Commit Bot 2010-01-20 20:07:13 PST
Comment on attachment 47082 [details]
removeAssert3

Clearing flags on attachment: 47082

Committed r53596: <http://trac.webkit.org/changeset/53596>
Comment 15 WebKit Commit Bot 2010-01-20 20:07:20 PST
All reviewed patches have been landed.  Closing bug.