Bug 149317 - REGRESSION (r182449, Mavericks ONLY): Pages re-open empty after swiping back and scrolling on them
Summary: REGRESSION (r182449, Mavericks ONLY): Pages re-open empty after swiping back ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-17 16:46 PDT by Chris Dumez
Modified: 2015-09-18 12:32 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.34 KB, patch)
2015-09-17 16:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2015-09-17 17:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (667.87 KB, application/zip)
2015-09-17 17:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (906.66 KB, application/zip)
2015-09-17 17:47 PDT, Build Bot
no flags Details
Patch (4.36 KB, patch)
2015-09-18 10:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-09-17 16:46:58 PDT
Pages re-open empty after swiping back and scrolling on them sometimes on Mavericks, after <http://trac.webkit.org/changeset/182449>.
Comment 1 Chris Dumez 2015-09-17 16:47:21 PDT
rdar://problem/22521514
Comment 2 Chris Dumez 2015-09-17 16:57:31 PDT
Created attachment 261459 [details]
Patch
Comment 3 Chris Dumez 2015-09-17 17:06:11 PDT
Created attachment 261460 [details]
Patch
Comment 4 Build Bot 2015-09-17 17:39:37 PDT
Comment on attachment 261460 [details]
Patch

Attachment 261460 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/181263

New failing tests:
http/tests/navigation/page-cache-pending-image-load.html
Comment 5 Build Bot 2015-09-17 17:39:41 PDT
Created attachment 261462 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 6 Build Bot 2015-09-17 17:47:42 PDT
Comment on attachment 261460 [details]
Patch

Attachment 261460 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/181273

New failing tests:
inspector/codemirror/prettyprinting-css-rules.html
http/tests/navigation/page-cache-pending-image-load.html
Comment 7 Build Bot 2015-09-17 17:47:45 PDT
Created attachment 261464 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 8 Alexey Proskuryakov 2015-09-17 18:01:20 PDT
Comment on attachment 261460 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        (images and XHR). This is because it has been determined via bisection
> +        that this change is the one that introduced the bug on Mavericks.

Is there any theory at all as to how this would be possible?
Comment 9 Chris Dumez 2015-09-17 18:27:09 PDT
(In reply to comment #8)
> Comment on attachment 261460 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261460&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        (images and XHR). This is because it has been determined via bisection
> > +        that this change is the one that introduced the bug on Mavericks.
> 
> Is there any theory at all as to how this would be possible?

No, I have no idea why at the moment. Since this is Mavericks only for now, I don't know if it is worth spending too much time figuring it out either.
Comment 10 Alexey Proskuryakov 2015-09-17 19:25:13 PDT
It's just not easy to imagine how the root cause can be Mavericks only.
Comment 11 Chris Dumez 2015-09-17 19:48:47 PDT
(In reply to comment #10)
> It's just not easy to imagine how the root cause can be Mavericks only.

I tend to agree. However, we have been able to reproduce this one fairly easily on Mavericks but never on more recent OSes.
Comment 12 Chris Dumez 2015-09-18 09:16:32 PDT
(In reply to comment #8)
> Comment on attachment 261460 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261460&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        (images and XHR). This is because it has been determined via bisection
> > +        that this change is the one that introduced the bug on Mavericks.
> 
> Is there any theory at all as to how this would be possible?

This particular bug only happens when swiping back, not when clicking the back button. Therefore, chances are this is the gesture snapshot being blank and not going away for some reason. I remember that when I introduced the optimization in r182449, I had to make a follow-up fix in <http://trac.webkit.org/changeset/184399> because the gesture snapshot would stay up (until the 3 seconds timeout) due to the page being marked as having an error (due to the cancelled image loads). It looks like this fix may have been insufficient on Mavericks.

I looked at the code to try and figure out what could be different on Mavericks and I would the following:
#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101000
#define ENABLE_LEGACY_SWIPE_SHADOW_STYLE 1
#endif

It looks like we use a slightly different code path for gesture snapshots on Mavericks so this could be related. Note however that this code has changed a lot since branching because Tim refactored the code so we share more between different platforms.

Now we could spend time trying to debug this Mavericks-only issue or we can simply disable the PageCache optimization that lets pages with cancelled loads (and thus in error state) into the PageCache. I would still vote for the latter.
Comment 13 Tim Horton 2015-09-18 09:20:52 PDT
(In reply to comment #12)
> (In reply to comment #8)
> > Comment on attachment 261460 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=261460&action=review
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        (images and XHR). This is because it has been determined via bisection
> > > +        that this change is the one that introduced the bug on Mavericks.
> > 
> > Is there any theory at all as to how this would be possible?
> 
> This particular bug only happens when swiping back, not when clicking the
> back button. Therefore, chances are this is the gesture snapshot being blank
> and not going away for some reason. I remember that when I introduced the
> optimization in r182449, I had to make a follow-up fix in
> <http://trac.webkit.org/changeset/184399> because the gesture snapshot would
> stay up (until the 3 seconds timeout) due to the page being marked as having
> an error (due to the cancelled image loads). It looks like this fix may have
> been insufficient on Mavericks.
> 
> I looked at the code to try and figure out what could be different on
> Mavericks and I would the following:
> #if __MAC_OS_X_VERSION_MIN_REQUIRED < 101000
> #define ENABLE_LEGACY_SWIPE_SHADOW_STYLE 1
> #endif
> 
> It looks like we use a slightly different code path for gesture snapshots on
> Mavericks so this could be related. Note however that this code has changed
> a lot since branching because Tim refactored the code so we share more
> between different platforms.
> 
> Now we could spend time trying to debug this Mavericks-only issue or we can
> simply disable the PageCache optimization that lets pages with cancelled
> loads (and thus in error state) into the PageCache. I would still vote for
> the latter.

I would love a CALayer tree dump when we're in this state...
Comment 14 Chris Dumez 2015-09-18 10:46:15 PDT
Created attachment 261506 [details]
Patch
Comment 15 Chris Dumez 2015-09-18 12:31:59 PDT
Comment on attachment 261506 [details]
Patch

Clearing flags on attachment: 261506

Committed r189976: <http://trac.webkit.org/changeset/189976>
Comment 16 Chris Dumez 2015-09-18 12:32:05 PDT
All reviewed patches have been landed.  Closing bug.