Bug 138061 - FrameProgressTracker expects Page to not have detached
Summary: FrameProgressTracker expects Page to not have detached
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vicki Pfau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-24 15:50 PDT by Vicki Pfau
Modified: 2014-10-28 15:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2014-10-24 15:52 PDT, Vicki Pfau
no flags Details | Formatted Diff | Diff
Patch (1.56 KB, patch)
2014-10-27 14:47 PDT, Vicki Pfau
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vicki Pfau 2014-10-24 15:50:57 PDT
Patch coming momentarily

<rdar://problem/18550647>
Comment 1 Vicki Pfau 2014-10-24 15:52:42 PDT
Created attachment 240443 [details]
Patch
Comment 2 Alexey Proskuryakov 2014-10-24 16:08:11 PDT
Comment on attachment 240443 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:192
> -        ASSERT(!m_inProgress || m_frame.page());
> -        if (m_inProgress)
> +        ASSERT(!m_inProgress);

This makes the ASSERT more strict, what guarantees that m_inProgress is always false here?

As it's not part of the fix, maybe the change can even be landed separately first, so that bots can report on it (EWS only runs release).
Comment 3 Vicki Pfau 2014-10-24 16:15:06 PDT
(In reply to comment #2)
> Comment on attachment 240443 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240443&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:192
> > -        ASSERT(!m_inProgress || m_frame.page());
> > -        if (m_inProgress)
> > +        ASSERT(!m_inProgress);
> 
> This makes the ASSERT more strict, what guarantees that m_inProgress is
> always false here?

Ah...that wasn't actually intentional. While it will usually be the case, the null pointer case I'm trying to fix will have m_inProgress be false. The assertion should be removed entirely, it seems.
Comment 4 Vicki Pfau 2014-10-24 16:16:51 PDT
I did run the full onslaught of tests, which didn't see it being false, but I have backtraces that show that it can be (as that's when the null pointer is relevant).
Comment 5 Darin Adler 2014-10-25 17:30:12 PDT
Comment on attachment 240443 [details]
Patch

review-; please post a patch without the assertion
Comment 6 Vicki Pfau 2014-10-27 14:47:53 PDT
Created attachment 240508 [details]
Patch
Comment 7 Alexey Proskuryakov 2014-10-28 00:14:03 PDT
Comment on attachment 240508 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No new tests; there is no behavior change, and it is impossible to
> +        reliably simulate the null pointer case without intrusive code changes.

I don't think that this explanation is accurate. I believe that you expect to fix a crash, which is a behavior change.

I assume that the reason why there is no patch is that we couldn't find a way to make one.
Comment 8 Vicki Pfau 2014-10-28 14:12:51 PDT
(In reply to comment #7)
> Comment on attachment 240508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240508&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests; there is no behavior change, and it is impossible to
> > +        reliably simulate the null pointer case without intrusive code changes.
> 
> I don't think that this explanation is accurate. I believe that you expect
> to fix a crash, which is a behavior change.
> 
> I assume that the reason why there is no patch is that we couldn't find a
> way to make one.

I can remove the "no behavior change" part, but the only way to simulate this would be to have a page that gets removed from the page cache before the load has committed, I believe, which I don't think we have a way of doing with layout tests.
Comment 9 Vicki Pfau 2014-10-28 15:38:42 PDT
Committed r175277: <http://trac.webkit.org/changeset/175277>