Bug 17667 - REGRESSION (r30587): Document::implicitClose() not called when done loading page (Image does not get scaled to fit)
Summary: REGRESSION (r30587): Document::implicitClose() not called when done loading p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: mitz
URL: http://factoryjoe.com/blog/2007/08/25...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-03-04 04:13 PST by Oliver Hunt
Modified: 2008-03-04 13:13 PST (History)
4 users (show)

See Also:


Attachments
Call setLoadInProgress(false) in the 4xx case (3.58 KB, patch)
2008-03-04 10:38 PST, mitz
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-03-04 04:13:56 PST
The image at the top of the post is not correctly scaled
Comment 1 Oliver Hunt 2008-03-04 04:22:12 PST
Looking in inspector one difference i noticed was that there was no listing for the img elements inline style...
Comment 2 mitz 2008-03-04 07:14:44 PST
The script library the page uses does not alter the inline style when running in Safari.

I hit this assertion failure when navigating away from this page:

void FrameView::scheduleRelayout()
{
    ASSERT(!m_frame->document() || !m_frame->document()->inPageCache());
Comment 3 mitz 2008-03-04 08:48:57 PST
I cannot reproduce either problem with a low-bandwidth connection.
Comment 4 mitz 2008-03-04 09:27:30 PST
Setting a breakpoint at Document::implicitClose() confirms that the load event is never dispatched when the page finishes loading, thus the script that's supposed to resize the images doesn't run. The load event *is* dispatched when you navigate out. By then the page is in the page cache and scripts really should not be executing. The event handler runs, resizes the image, and triggers a layout of a now-cached page, which causes the assertion failure.
Comment 5 mitz 2008-03-04 09:30:45 PST
Indeed, you can force the images to resize by entering
javascript: document.close()
in the address bar after the page has finished loading.
Comment 6 mitz 2008-03-04 10:13:45 PST
The page links to a 404 style sheet, <http://factoryjoe.com/blog/wp-content/plugins/diso-profile/profile.css>, so I highly suspect <http://trac.webkit.org/projects/webkit/changeset/30587>.
Comment 7 mitz 2008-03-04 10:29:58 PST
<rdar://problem/5779658>
Comment 8 mitz 2008-03-04 10:38:15 PST
Created attachment 19526 [details]
Call setLoadInProgress(false) in the 4xx case
Comment 9 Oliver Hunt 2008-03-04 12:03:06 PST
Comment on attachment 19526 [details]
Call setLoadInProgress(false) in the 4xx case

r=me, though what's with "\ No newline at end of file" in the middle of the testcase?
Comment 10 Geoffrey Garen 2008-03-04 12:04:25 PST
r=me too, but I noticed another potential problem: do we need to surround the call to object->error() inside Loader::didReceiveData with calls to setLoadInProgress(true) and then setLoadInProgress(false)?
Comment 11 mitz 2008-03-04 12:16:57 PST
(In reply to comment #10)
> r=me too, but I noticed another potential problem: do we need to surround the
> call to object->error() inside Loader::didReceiveData with calls to
> setLoadInProgress(true) and then setLoadInProgress(false)?

I think so but I do not know how to make a test case to prove it.
Comment 12 mitz 2008-03-04 12:18:05 PST
(In reply to comment #9)
> (From update of attachment 19526 [details] [edit])
> r=me, though what's with "\ No newline at end of file" in the middle of the
> testcase?

There used not to be a newline, now there should be.
Comment 13 mitz 2008-03-04 13:13:57 PST
Landed in <http://trac.webkit.org/projects/webkit/changeset/30762>.