Bug 41178 – Timed refresh in subframes isn't stopped when going into b/f cache

• Component: Frames
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Loading inside a cached frame causes all sorts of crashes. Patch forthcoming.

[Alexey Proskuryakov]

Created attachment 59706 [details]
proposed fix

Let's dupe other bugs that are likely caused by this problem to this one.

[Alexey Proskuryakov]

<rdar://problem/8128955>

[Brady Eidson]

Comment on attachment 59706 [details]
proposed fix


>  
> Index: WebCore/loader/FrameLoader.cpp
> ===================================================================
> --- WebCore/loader/FrameLoader.cpp	(revision 61698)
> +++ WebCore/loader/FrameLoader.cpp	(working copy)
> @@ -491,10 +491,7 @@ void FrameLoader::stopLoading(UnloadEven
>  #endif
>      }
>  
> -    // tell all subframes to stop as well
> -    for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
> -        child->loader()->stopLoading(unloadEventPolicy);
> -
> +    // FIXME: This will cancel redirection timer, which really needs to be restarted when restoring the frame from b/f cache.
>      m_frame->redirectScheduler()->cancel();

Don't we restore redirect timers for the main frame when restoring from the page cache?

If so, is it acceptable to have this outstanding FIXME for subframes?


>  
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 61786)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Alexey Proskuryakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=41178
> +        Timed refresh in subframes isn't stopped when going into b/f cache
> +
> +        This is a slow test, because a fast redirect results in replacing the current item in b/f
> +        list, so back/forward cache doesn't get involved. But this code path must be tested.
> +

Even if it's an inconvience to everyone while folks are still working at parallelizing and otherwise speeding up the layouttests, I agree we absolutely need this type of test, even if it's slow.

> +        * fast/history/timed-refresh-in-cached-frame-expected.txt: Added.
> +        * fast/history/timed-refresh-in-cached-frame.html: Added.

Maybe we should to create a new TLD in LayoutTests 'slow' for this type of test?

r- because of my question on the FIXME

[Geoffrey Garen]

Looks good to me, modulo Brady's comments.

[Alexey Proskuryakov]

Comment on attachment 59706 [details]
proposed fix

> Don't we restore redirect timers for the main frame when restoring from the page cache?

We don't. Pages that come back from page cache are almost dead, see semi-related bug 34679.

> Maybe we should to create a new TLD in LayoutTests 'slow' for this type of test?

I don't think it would be helpful. The reason to have multiple directories is that one can run only a subset of tests pertinent to the code area while iteratively developing a patch (e.g., I've been running fast/history, fast/events and fast/frames for this patch).

And this test isn't the slowest that we have!

[Brady Eidson]

Comment on attachment 59706 [details]
proposed fix

Okay, you got me.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/61801>.

[Alexey Proskuryakov]

*** Bug 41028 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 41120 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 59706 [details]
proposed fix

Looks good.