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.
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).
See also: <rdar://problem/30180098>
(In reply to comment #2) > See also: > <rdar://problem/30180098> and <rdar://problem/30188490>
(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.
(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.
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.
Created attachment 299721 [details] WIP patch
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.
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 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.
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 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.
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
Created attachment 299729 [details] WIP patch
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
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 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
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 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
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
Created attachment 299741 [details] WIP patch
Created attachment 299748 [details] Patch
Created attachment 299755 [details] Patch
Can we remove the code added in <http://trac.webkit.org/changeset/210474> (bug #166773)?
(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?
(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.
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?
Created attachment 299767 [details] Patch
Created attachment 299784 [details] Patch
EWS is green, patch is ready for review.
Created attachment 299813 [details] Patch
(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 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 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
(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.
Created attachment 299905 [details] Patch
Comment on attachment 299905 [details] Patch Clearing flags on attachment: 299905 Committed r211254: <http://trac.webkit.org/changeset/211254>
All reviewed patches have been landed. Closing bug.
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.