Bug 167421 – Crash when navigating back to a page in PacheCache when one of its frames has been removed

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

[Chris Dumez]

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.

[Chris Dumez]

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

[Chris Dumez]

See also:
<rdar://problem/30180098>

[Chris Dumez]

(In reply to comment #2)
> See also:
> <rdar://problem/30180098>

and <rdar://problem/30188490>

[Brady Eidson]

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

[Brady Eidson]

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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 299721 [details]
WIP patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Chris Dumez]

Created attachment 299729 [details]
WIP patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

Created attachment 299741 [details]
WIP patch

[Chris Dumez]

Created attachment 299748 [details]
Patch

[Chris Dumez]

Created attachment 299755 [details]
Patch

[Daniel Bates]

Can we remove the code added in <http://trac.webkit.org/changeset/210474> (bug #166773)?

[Chris Dumez]

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

[Daniel Bates]

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

[Daniel Bates]

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?

[Chris Dumez]

Created attachment 299767 [details]
Patch

[Chris Dumez]

Created attachment 299784 [details]
Patch

[Chris Dumez]

EWS is green, patch is ready for review.

[Chris Dumez]

Created attachment 299813 [details]
Patch

[Chris Dumez]

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

[Darin Adler]

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.

[WebKit Commit Bot]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 299905 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 299905 [details]
Patch

Clearing flags on attachment: 299905

Committed r211254: <http://trac.webkit.org/changeset/211254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

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.