Bug 156356

Summary: pageshow only fires the first time the back button is pressed
Product: WebKit Reporter: Jeff Carlsen <trillinon>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andre, ap, beidson, cdumez, evandergraaf, ews-watchlist, ggaren, japhet, kimatg, paulo.vinicius117, semper.cras, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari 9   
Hardware: Mac   
OS: OS X 10.11   
See Also: https://bugs.webkit.org/show_bug.cgi?id=18472
https://bugs.webkit.org/show_bug.cgi?id=59111
https://bugs.webkit.org/show_bug.cgi?id=74402
https://bugs.webkit.org/show_bug.cgi?id=213657
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Jeff Carlsen 2016-04-07 13:09:18 PDT
Repro steps:

1. Go to page A
2. Click a link to page B
3. Click the back button (pageshow fires as expected)
4. Click a link to page B again (or any other page)
5. Click the back button (pageshow does not fire)

I discovered this because on the page I'm working on, I want to fetch some fresh data when they return to a page through the back button. I do this by listening for pageshow and checking the event.persisted property.
Comment 1 Alexey Proskuryakov 2016-04-07 18:27:18 PDT
Could you please provide a test case, or a link to a test page? I'm pretty sure that I tested this some time before, and it worked as expected, so it may be tricky to reproduce from a description alone.
Comment 2 Paulo Pereira 2016-09-16 04:30:21 PDT
Hi, I don't know if it's the exact same situation that is happening to Jeff Carlsen. But I am having the exact same behavior and I noticed this is also happening to "pagehide" event.

After some heavy debugging I figured out why this is happening and I created this test case example reproducing it: 

1. Go to http://paulovpereira.github.io/pageshow-webkit/ in an IOS device (I tested in ios 9 and 10)
2. Click in the navigate link
3. press back button
4. press forward button
5. keep going back and forth and check the events being triggered and logged in the body

The only difference between the pages is the existence of an iframe element and this is causing the event not to be fired. I hope this help figuring out how to fix the bug.
Comment 3 Georgii Ivankin 2016-11-14 14:35:23 PST
Second this. Just having 2 pages, both with 

window.addEventListener("pageshow", function(event) {
    console.info('pageshow. persisted: ' + event.persisted);
});

and navigating back-forward between them, is enough to reproduce. 

People has been reporting this on SO as well: http://stackoverflow.com/q/10106457/697388 
http://stackoverflow.com/q/33804292/697388
http://stackoverflow.com/questions/11979156/mobile-safari-back-button#comment44257718_12652160


Version 10.0.1 (12602.2.14.0.7) on MacOS Sierra.
Comment 4 Radar WebKit Bug Importer 2016-11-14 15:39:58 PST
<rdar://problem/29256489>
Comment 5 Lucas Forschler 2019-02-06 09:18:40 PST
Mass move bugs into the DOM component.
Comment 6 Hansol Kim 2020-05-25 08:44:54 PDT
Hi, I just stumbled upon this issue after struggling hours trying to find a possible workaround but could find none so far. After discovering there's already a bug filed for this issue, I'm surprised nothing happened even after 4 years now... are there any updates regarding this or plans to fix?
Comment 7 Chris Dumez 2020-05-27 12:04:28 PDT
(In reply to Hansol Kim from comment #6)
> Hi, I just stumbled upon this issue after struggling hours trying to find a
> possible workaround but could find none so far. After discovering there's
> already a bug filed for this issue, I'm surprised nothing happened even
> after 4 years now... are there any updates regarding this or plans to fix?

Thanks for bringing this to our attention. It looks like this bug fell through the cracks. The bug indeed still reproduces in the latest WebKit and I am looking into it.
Comment 8 Chris Dumez 2020-05-27 12:46:37 PDT
Created attachment 400366 [details]
Patch
Comment 9 Chris Dumez 2020-05-27 13:17:19 PDT
Created attachment 400372 [details]
Patch
Comment 10 Chris Dumez 2020-05-27 13:19:09 PDT
Comment on attachment 400372 [details]
Patch

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

> LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt:38
> +http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didFinishLoading

This is a progression, it makes back/forward navigations more consistent with regular navigations. For e.g., if you look at LayoutTests/http/tests/loading/redirect-methods-expected.txt:
"""
main frame - didFinishLoadForFrame
http://127.0.0.1:8000/loading/redirect-methods.html - didFinishLoading
"""

We normally call didFinishLoadForFrame for the main frame *before* calling * didFinishLoading*, which makes more sense.
Comment 11 Alexey Proskuryakov 2020-05-27 13:46:23 PDT
Comment on attachment 400372 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * fast/history/resources/page-cache-helper.html:

Comment?

>> LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt:38
>> +http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didFinishLoading
> 
> This is a progression, it makes back/forward navigations more consistent with regular navigations. For e.g., if you look at LayoutTests/http/tests/loading/redirect-methods-expected.txt:
> """
> main frame - didFinishLoadForFrame
> http://127.0.0.1:8000/loading/redirect-methods.html - didFinishLoading
> """
> 
> We normally call didFinishLoadForFrame for the main frame *before* calling * didFinishLoading*, which makes more sense.

Progressions of this kind are scary, as clients rely on quirks like this. But I don't know what kind of testing we could to qualify the fix do besides automated tests.
Comment 12 Chris Dumez 2020-05-27 14:03:04 PDT
Created attachment 400377 [details]
Patch
Comment 13 Chris Dumez 2020-05-27 14:03:24 PDT
(In reply to Alexey Proskuryakov from comment #11)
> Comment on attachment 400372 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400372&action=review
> 
> > LayoutTests/ChangeLog:13
> > +        * fast/history/resources/page-cache-helper.html:
> 
> Comment?

I added an explanation.

> 
> >> LayoutTests/http/tests/loading/main-resource-delegates-on-back-navigation-expected.txt:38
> >> +http://127.0.0.1:8000/loading/main-resource-delegates-on-back-navigation.html - didFinishLoading
> > 
> > This is a progression, it makes back/forward navigations more consistent with regular navigations. For e.g., if you look at LayoutTests/http/tests/loading/redirect-methods-expected.txt:
> > """
> > main frame - didFinishLoadForFrame
> > http://127.0.0.1:8000/loading/redirect-methods.html - didFinishLoading
> > """
> > 
> > We normally call didFinishLoadForFrame for the main frame *before* calling * didFinishLoading*, which makes more sense.
> 
> Progressions of this kind are scary, as clients rely on quirks like this.
> But I don't know what kind of testing we could to qualify the fix do besides
> automated tests.
Comment 14 Chris Dumez 2020-05-27 14:29:07 PDT
Created attachment 400383 [details]
Patch
Comment 15 Chris Dumez 2020-05-27 14:30:35 PDT
Resetting review flag since I had to make a small change to make API test happy. This change is more conservative, we call checkCompleted() for subframes in FrameLoader::open(). For the main frame, we keep calling checkCompleted() where we used to to avoid behavior changes.
Comment 16 EWS 2020-05-27 15:49:25 PDT
Committed r262221: <https://trac.webkit.org/changeset/262221>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400383 [details].