Bug 167421

Summary: Crash when navigating back to a page in PacheCache when one of its frames has been removed
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Page LoadingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, bfulgham, buildbot, commit-queue, darin, dbates, esprehn+autocc, ggaren, kangil.han, koivisto, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 167487, 168928    
Attachments:
Description Flags
WIP patch
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
WIP patch
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews117 for mac-elcapitan
none
WIP patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-01-25 09:14:46 PST
Crash when navigating back to a page in PacheCache when one of its frames has been removed:
    let w = open("about:blank", "one");
    let d = w.document;
    let f = d.createElement("iframe");
    f.src = "http://www.espn.com";
    d.body.appendChild(f);

    let a = d.createElement("a");
    a.href = "https://google.com";
    a.click();

    let it = setInterval(() => {
        clearInterval(it);
        d.body.innerHTML = "LOL";
    }, 2000);


Wait 3 seconds then navigate back in the newly opened Window (that currently shows google.com). You'll get the following crash:
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000100
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

Application Specific Information:
This process is running with libgmalloc.dylib (GuardMalloc) which may have forced the crash due to a memory access error.
 
Bundle controller class:
BrowserBundleController

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000109e924fe WebCore::CachedFrameBase::restore() + 222 (Page.h:192)
1   com.apple.WebCore             	0x000000010a1e9ea5 WebCore::FrameLoader::open(WebCore::CachedFrameBase&) + 789 (utility:753)
2   com.apple.WebCore             	0x0000000109e96519 WebCore::CachedPage::restore(WebCore::Page&) + 25 (memory:2701)
3   com.apple.WebCore             	0x000000010a1e8637 WebCore::FrameLoader::commitProvisionalLoad() + 775 (Optional.h:366)
4   com.apple.WebCore             	0x000000010a1e6a9c WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL) + 1180 (FrameLoader.cpp:3251)
5   com.apple.WebCore             	0x000000010aa5c41d WebCore::PolicyCallback::call(bool) + 61 (functional:1817)
6   com.apple.WebCore             	0x000000010aa5d18c WebCore::PolicyChecker::continueAfterNavigationPolicy(WebCore::PolicyAction) + 732 (PolicyCallback.h:47)
7   com.apple.WebKit              	0x0000000107d0dbcc WebKit::WebFrame::didReceivePolicyDecision(unsigned long long, WebCore::PolicyAction, unsigned long long, WebKit::DownloadID) + 192 (functional:1766)
8   com.apple.WebKit              	0x0000000107d12330 WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&, WebCore::ResourceRequest const&, WebCore::FormState*, std::__1::function<void (WebCore::PolicyAction)>) + 1590 (WebFrameLoaderClient.cpp:822)
9   com.apple.WebCore             	0x000000010aa5ce11 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, std::__1::function<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>) + 1937 (functional:1766)
10  com.apple.WebCore             	0x000000010a1e63d2 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL) + 2258 (functional:1766)
11  com.apple.WebCore             	0x000000010a1e2489 WebCore::FrameLoader::loadDifferentDocumentItem(WebCore::HistoryItem&, WebCore::FrameLoadType, WebCore::FrameLoader::FormSubmissionCacheLoadPolicy) + 409 (FrameLoader.cpp:3321)
12  com.apple.WebCore             	0x000000010a2624df WebCore::HistoryController::goToItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 207 (HistoryController.cpp:325)
13  com.apple.WebCore             	0x000000010aa1514a WebCore::Page::goToItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 90 (RefCounted.h:98)
14  com.apple.WebKit              	0x0000000107d4ca98 WebKit::WebPage::goBack(unsigned long long, unsigned long long) + 54 (MessageSender.h:39)

This is because it is currently possible for an opener to have wrappers to Nodes/Documents in windows that in opened (via window.open) and then for those documents to go into PageCache. Doing DOM mutations on documents that are currently in PageCache is unsupported and will lead to various crashes.
Comment 1 Chris Dumez 2017-01-25 09:23:53 PST
My short-term proposal would be to lock this down by preventing page-caching if:
1. The window has an opener (because that opener might script us after we go into the cache)
2. The window has ever called window.open() (because those windows might script their opener and that opener's document may enter PageCache on navigation). <- Thanks Alexey for pointing this out.

We may be able to come up with a more precise way of detecting that another window can script a document that is about to enter PageCache (i.e. that other window hold a wrapper to the about-to-be-cached document or one of its Nodes). But short term, this would lock prevent this class of bugs.

If we allow page caching here, we would have to somehow make sure that:
- We never run script in that PageCached document
- DOM mutations to PageCached documents do not cause crashes (May end up being difficult because this is not something we have handled in the past and I suspect there is a lot of code that will not deal properly with mutating the tree while in PageCache).
Comment 2 Chris Dumez 2017-01-25 09:24:57 PST
See also:
<rdar://problem/30180098>
Comment 3 Chris Dumez 2017-01-25 09:53:00 PST
(In reply to comment #2)
> See also:
> <rdar://problem/30180098>

and <rdar://problem/30188490>
Comment 4 Brady Eidson 2017-01-25 10:51:04 PST
(In reply to comment #3)
> (In reply to comment #2)
> > See also:
> > <rdar://problem/30180098>
> 
> and <rdar://problem/30188490>


And <rdar://problem/29904368>

CC'ing Dan Bates, whom with I talked about this very issue yesterday.
Comment 5 Brady Eidson 2017-01-25 10:51:44 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > See also:
> > > <rdar://problem/30180098>
> > 
> > and <rdar://problem/30188490>
> 
> 
> And <rdar://problem/29904368>
> 
> CC'ing Dan Bates, whom with I talked about this very issue yesterday.

To be more precise - We talked about a superset of issues that includes this one.
Comment 6 Chris Dumez 2017-01-25 11:34:05 PST
Discussed this with Brady/Brent/Antti/Daniel and it looks like we have agreement to disable PageCache in this case, as a short term fix, while Daniel investigates better solutions.

I'll upload a patch today.
Comment 7 Chris Dumez 2017-01-25 11:37:02 PST
Created attachment 299721 [details]
WIP patch
Comment 8 Build Bot 2017-01-25 12:17:44 PST
Comment on attachment 299721 [details]
WIP patch

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

Number of test failures exceeded the failure limit.
Comment 9 Build Bot 2017-01-25 12:17:48 PST
Created attachment 299724 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-01-25 12:31:46 PST
Comment on attachment 299721 [details]
WIP patch

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

Number of test failures exceeded the failure limit.
Comment 11 Build Bot 2017-01-25 12:31:52 PST
Created attachment 299726 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-01-25 12:36:59 PST
Comment on attachment 299721 [details]
WIP patch

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

Number of test failures exceeded the failure limit.
Comment 13 Build Bot 2017-01-25 12:37:04 PST
Created attachment 299727 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 14 Chris Dumez 2017-01-25 12:41:14 PST
Created attachment 299729 [details]
WIP patch
Comment 15 Build Bot 2017-01-25 13:52:47 PST
Comment on attachment 299729 [details]
WIP patch

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

New failing tests:
editing/mac/input/unconfirmed-text-navigation-with-page-cache.html
fast/harness/use-page-cache.html
fast/loader/stateobjects/no-popstate-when-back-to-stateless-entry-with-page-cache.html
fast/loader/stateobjects/popstate-fires-with-page-cache.html
fast/harness/page-cache-crash-on-data-urls.html
Comment 16 Build Bot 2017-01-25 13:52:52 PST
Created attachment 299736 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-01-25 14:00:51 PST
Comment on attachment 299729 [details]
WIP patch

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

New failing tests:
fast/loader/stateobjects/popstate-fires-with-page-cache.html
fast/loader/stateobjects/no-popstate-when-back-to-stateless-entry-with-page-cache.html
tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
Comment 18 Build Bot 2017-01-25 14:00:55 PST
Created attachment 299739 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-01-25 14:04:10 PST
Comment on attachment 299729 [details]
WIP patch

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

New failing tests:
editing/mac/input/unconfirmed-text-navigation-with-page-cache.html
fast/harness/use-page-cache.html
fast/loader/stateobjects/no-popstate-when-back-to-stateless-entry-with-page-cache.html
fast/loader/stateobjects/popstate-fires-with-page-cache.html
fast/harness/page-cache-crash-on-data-urls.html
Comment 20 Build Bot 2017-01-25 14:04:14 PST
Created attachment 299740 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Chris Dumez 2017-01-25 14:04:47 PST
Created attachment 299741 [details]
WIP patch
Comment 22 Chris Dumez 2017-01-25 14:41:41 PST
Created attachment 299748 [details]
Patch
Comment 23 Chris Dumez 2017-01-25 15:30:26 PST
Created attachment 299755 [details]
Patch
Comment 24 Daniel Bates 2017-01-25 15:55:00 PST
Can we remove the code added in <http://trac.webkit.org/changeset/210474> (bug #166773)?
Comment 25 Chris Dumez 2017-01-25 16:00:38 PST
(In reply to comment #24)
> Can we remove the code added in <http://trac.webkit.org/changeset/210474>
> (bug #166773)?

I was planning to do it in a follow-up. Does it need to be in the same patch?
Comment 26 Daniel Bates 2017-01-25 16:15:05 PST
(In reply to comment #25)
> (In reply to comment #24)
> > Can we remove the code added in <http://trac.webkit.org/changeset/210474>
> > (bug #166773)?
> 
> I was planning to do it in a follow-up. Does it need to be in the same patch?

No, it does not need to be in the same patch.
Comment 27 Daniel Bates 2017-01-25 16:36:18 PST
For your consideration, I suggest that we add a remark in the ChangeLog description to explain that a side effect of disallowing a page to be put into the page cache is that any modifications to such a page made by the opener or openee will be lost. You may even want to call out the implications of navigating back to a completely dynamically generated page (we will show a blank page as opposed to the last state of the page (*)). This will help explain to web developers that run into this side effect that it is intentional.

(*) Could we/would it make sense to detect this case and inform the embedding client to disable the back button to prevent navigating to a blank page?
Comment 28 Chris Dumez 2017-01-25 16:42:48 PST
Created attachment 299767 [details]
Patch
Comment 29 Chris Dumez 2017-01-25 19:00:13 PST
Created attachment 299784 [details]
Patch
Comment 30 Chris Dumez 2017-01-25 19:54:08 PST
EWS is green, patch is ready for review.
Comment 31 Chris Dumez 2017-01-26 09:44:25 PST
Created attachment 299813 [details]
Patch
Comment 32 Chris Dumez 2017-01-26 09:45:14 PST
(In reply to comment #31)
> Created attachment 299813 [details]
> Patch

I added an extra test that reproduces the particular crash in this bug, for good measure.
Comment 33 Darin Adler 2017-01-26 21:00:37 PST
Comment on attachment 299813 [details]
Patch

Seems a little peculiar to add a setting. I’m also interested in what a better long term solution to this would be.
Comment 34 WebKit Commit Bot 2017-01-26 21:02:02 PST
Comment on attachment 299813 [details]
Patch

Rejecting attachment 299813 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 299813, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
y/resources/page-cache-window-with-opener.html
patching file LayoutTests/fast/loader/stateobjects/no-popstate-when-back-to-stateless-entry-with-page-cache.html
patching file LayoutTests/fast/loader/stateobjects/popstate-fires-with-page-cache.html
patching file LayoutTests/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2956257
Comment 35 Chris Dumez 2017-01-26 21:06:03 PST
(In reply to comment #33)
> Comment on attachment 299813 [details]
> Patch
> 
> Seems a little peculiar to add a setting. I’m also interested in what a
> better long term solution to this would be.

I discussed this with Geoff and Daniel Bates today. It sounds like we'd like a document that is in PageCache to behave the same way as a detached/frameless document.

This would probably involve something like:
1. Clearing m_frame pointer on Document while it is in the PageCache and resetting m_frame to its original value when restoring the Document from PageCache
2. Make sure script is not run when inserted into such document (clearing m_frame might already take care of this)
3. Make PageCache restore robust to DOM mutations while in the cache.
Comment 36 Chris Dumez 2017-01-26 21:10:17 PST
Created attachment 299905 [details]
Patch
Comment 37 WebKit Commit Bot 2017-01-26 21:37:40 PST
Comment on attachment 299905 [details]
Patch

Clearing flags on attachment: 299905

Committed r211254: <http://trac.webkit.org/changeset/211254>
Comment 38 WebKit Commit Bot 2017-01-26 21:37:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 Brent Fulgham 2017-02-27 13:42:56 PST
Comment on attachment 299905 [details]
Patch

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

> Source/WebKit/win/WebPreferences.h:163
> +    virtual HRESULT STDMETHODCALLTYPE setAllowsPageCacheWithWindowOpener(BOOL);

Note: This change breaks existing WebKit.dll clients. Since this is a COM interface, we cannot add new methods without introducing a new interface ID.

We should create a new "IWebPreferencesPrivate5" interface with these two methods, and move down lower in the class to avoid breaking binary compatibility.