Bug 138061 – FrameProgressTracker expects Page to not have detached

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Vicki Pfau]

Patch coming momentarily

<rdar://problem/18550647>

[Vicki Pfau]

Created attachment 240443 [details]
Patch

[Alexey Proskuryakov]

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

[Vicki Pfau]

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

[Vicki Pfau]

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

[Darin Adler]

Comment on attachment 240443 [details]
Patch

review-; please post a patch without the assertion

[Vicki Pfau]

Created attachment 240508 [details]
Patch

[Alexey Proskuryakov]

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.

[Vicki Pfau]

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

[Vicki Pfau]

Committed r175277: <http://trac.webkit.org/changeset/175277>