Bug 94143 – Assertion going back to results.html page from an image diff result

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Simon Fraser (smfr)]

I assert every time I go Back to the results.html page after examining a ref test result image in the standard layout test results page.

We assert here:

    ASSERT(!window() || domWindow.get() != window()->impl());

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010a3b631e WebCore::JSDOMWindowShell::setWindow(WTF::PassRefPtr<WebCore::DOMWindow>) + 142 (JSDOMWindowShell.cpp:69)
1   com.apple.WebCore             	0x000000010ac72862 WebCore::ScriptCachedFrameData::restore(WebCore::Frame*) + 402 (ScriptCachedFrameData.cpp:83)
2   com.apple.WebCore             	0x0000000109953080 WebCore::CachedFrameBase::restore() + 240 (CachedFrame.cpp:103)
3   com.apple.WebCore             	0x0000000109ee3a49 WebCore::FrameLoader::open(WebCore::CachedFrameBase&) + 1033 (FrameLoader.cpp:1955)
4   com.apple.WebCore             	0x0000000109953417 WebCore::CachedFrame::open() + 183 (CachedFrame.cpp:212)
5   com.apple.WebCore             	0x0000000109959fa9 WebCore::CachedPage::restore(WebCore::Page*) + 377 (CachedPage.cpp:83)
6   com.apple.WebCore             	0x0000000109ee1f03 WebCore::FrameLoader::commitProvisionalLoad() + 1267 (FrameLoader.cpp:1685)
7   com.apple.WebCore             	0x0000000109ee57d7 WebCore::FrameLoader::loadProvisionalItemFromCachedPage() + 311 (FrameLoader.cpp:2910)
8   com.apple.WebCore             	0x0000000109ee061b WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) + 843 (FrameLoader.cpp:2779)
9   com.apple.WebCore             	0x0000000109ee0717 WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void*, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) + 87 (FrameLoader.cpp:2655)
10  com.apple.WebCore             	0x000000010a94aaf2 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, void (*)(void*, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool), void*) + 370 (PolicyChecker.cpp:69)
11  com.apple.WebCore             	0x0000000109ee00eb WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>) + 1467 (FrameLoader.cpp:1363)
12  com.apple.WebCore             	0x0000000109edc351 WebCore::FrameLoader::loadDifferentDocumentItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 161 (FrameLoader.cpp:3007)
13  com.apple.WebCore             	0x0000000109ee62b1 WebCore::FrameLoader::loadItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 161 (FrameLoader.cpp:3095)
14  com.apple.WebCore             	0x0000000109f9fe85 WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem*, WebCore::HistoryItem*, WebCore::FrameLoadType) + 565 (HistoryController.cpp:736)
15  com.apple.WebCore             	0x0000000109f9f9fe WebCore::HistoryController::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 430 (HistoryController.cpp:275)
16  com.apple.WebCore             	0x000000010a8f16e9 WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 201 (Page.cpp:367)
17  com.apple.WebKit2             	0x0000000107c57237 WebKit::WebPage::goBack(unsigned long long) + 183 (WebPage.cpp:830)
18  com.apple.WebKit2             	0x0000000107c8d12a void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long), unsigned long long>(CoreIPC::Arguments1<unsigned long long> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long)) + 138 (HandleMessage.h:20)
19  com.apple.WebKit2             	0x0000000107c83dbf void CoreIPC::handleMessage<Messages::WebPage::GoBack, WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long long)>(CoreIPC::ArgumentDecoder*, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long long)) + 111 (HandleMessage.h:303)
20  com.apple.WebKit2             	0x0000000107c817a2 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 850 (WebPageMessageReceiver.cpp:153)
21  com.apple.WebKit2             	0x0000000107c5c09d WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 301 (WebPage.cpp:2675)
22  com.apple.WebKit2             	0x0000000107d11dbb WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 923 (WebProcess.cpp:716)
23  com.apple.WebKit2             	0x0000000107bb4e5e WebKit::WebConnectionToUIProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 350 (WebConnectionToUIProcess.cpp:88)
24  com.apple.WebKit2             	0x0000000107bb4ead non-virtual thunk to WebKit::WebConnectionToUIProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 61
25  com.apple.WebKit2             	0x0000000107a53c4c CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::ArgumentDecoder>&) + 348 (Connection.cpp:691)
26  com.apple.WebKit2             	0x0000000107a563fb CoreIPC::Connection::dispatchOneMessage() + 203 (Connection.cpp:718)
27  com.apple.WebKit2             	0x0000000107a5cf02 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*) + 114 (Functional.h:173)
28  com.apple.WebKit2             	0x0000000107a5ce85 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()() + 53 (Functional.h:405)
29  com.apple.WebCore             	0x000000010ac5d565 WTF::Function<void ()>::operator()() const + 133 (Functional.h:613)
30  com.apple.WebCore             	0x000000010ac5d1af WebCore::RunLoop::performWork() + 207 (RunLoop.cpp:89)
31  com.apple.WebCore             	0x000000010ac5e69e WebCore::RunLoop::performWork(void*) + 62 (RunLoopCF.cpp:66)

[Radar WebKit Bug Importer]

<rdar://problem/12106883>

[Brady Eidson]

When did this start?

[Radar WebKit Bug Importer]

<rdar://problem/12108266>

[Simon Fraser (smfr)]

(lldb) expr window()
(WebCore::JSDOMWindow *) $1 = 0x0000000152f9d880
(lldb) p domWindow.get()
(WebCore::DOMWindow *) $2 = 0x00007fcb29966f50
(lldb) p window()->impl()
(WebCore::DOMWindow *) $3 = 0x00007fcb29966f50

[Adam Barth]

I'm having trouble reproducing this issue with a build at r125682.

[Adam Barth]

Exact repro steps:

---8<---
 ./Tools/Scripts/run-safari --debug about:blank
navigate to http://www.webkit.org/
click "download the lastest nightly build"
click the back button?
--->8---

smfr confirms that it doesn't repro with release Safari but it does repro with an Apple-internal build of Safari.

[Adam Barth]

Created attachment 158664 [details]
A sketch of a layout test (does not assert)

[Adam Barth]

smfr, I'm sorry but I'm not sure how to make progress here.  I would like to help resolve the issue, but it's difficult without being able to reproduce the issue.

Do you know which half of the ASSERT is triggering?

    ASSERT(!window() || domWindow.get() != window()->impl());

i.e., is is !window() or is it domWindow.get() != window()->impl() ?

[Adam Barth]

I guess Comment #4 says that it's the second half.

[Adam Barth]

A blind guess at a fix is to teach ScriptCachedFrameData::restore not to call JSDOMWindowShell::setWindow with the same DOMWindow, but without understanding why it's doing that, I'm not super confident in that solution.

[Brady Eidson]

The interesting thing to me is that this is (apparently) a recent regression.  I'd really like to see what change broke things.

[Adam Barth]

> The interesting thing to me is that this is (apparently) a recent regression.  I'd really like to see what change broke things.

It's likely to be http://trac.webkit.org/changeset/125592

[Simon Fraser (smfr)]

I can't get any work done because of this. Will look now.

[Simon Fraser (smfr)]

Series of setWindow calls leading up to the assertion:

JSDOMWindowShell 0x108fffe80 setWindow 0x10b23cad0, window 0x10f91d880, window impl 0x10016a360
JSDOMWindowShell 0x108ffffc0 setWindow 0x10b23cad0, window 0x10f91d380, window impl 0x10016a360
JSDOMWindowShell 0x108fff3c0 setWindow 0x10b23cad0, window 0x10f91c480, window impl 0x10016a360
JSDOMWindowShell 0x108fff340 setWindow 0x10b23cad0, window 0x10f91bf80, window impl 0x10016a360
JSDOMWindowShell 0x108ffff00 setWindow 0x10b23cad0, window 0x10f91ce80, window impl 0x10016a360
JSDOMWindowShell 0x108fffe00 setWindow 0x10b23cad0, window 0x10f91c980, window impl 0x10016a360
JSDOMWindowShell 0x108fff3c0 setWindow 0x10b23cad0, window 0x10f91b580, window impl 0x10b23cad0

[Simon Fraser (smfr)]

The first relevant setWindow call comes from:

* thread #1: tid = 0x1f03, 0x00000001039bdbd7 WebCore`WebCore::JSDOMWindowShell::setWindow(WTF::PassRefPtr<WebCore::DOMWindow>) + 167 at JSDOMWindowShell.cpp:70, stop reason = breakpoint 2.1
    frame #0: 0x00000001039bdbd7 WebCore`WebCore::JSDOMWindowShell::setWindow(WTF::PassRefPtr<WebCore::DOMWindow>) + 167 at JSDOMWindowShell.cpp:70
    frame #1: 0x0000000104286f33 WebCore`WebCore::ScriptController::clearWindowShell(WebCore::DOMWindow*, bool) + 323 at ScriptController.cpp:195
    frame #2: 0x00000001034df199 WebCore`WebCore::FrameLoader::clear(WebCore::Document*, bool, bool, bool) + 425 at FrameLoader.cpp:528

and the second, with the identical window, from  ScriptCachedFrameData::restore().

[Simon Fraser (smfr)]

Created attachment 159559 [details]
Patch

[Brady Eidson]

Comment on attachment 159559 [details]
Patch

Is it okay to just bypass the issue without understanding what introduced it?

[Adam Barth]

Comment on attachment 159559 [details]
Patch

Thanks for fixing this bug.  I'm sorry that I could not fix it myself.

[Adam Barth]

> Is it okay to just bypass the issue without understanding what introduced it?

The core thing that changes is that we're not explicitly re-using DOMWindow objects in some cases (e.g., XSLT and initial documents that do a "secure transition to" the next document).  Previously, this all worked because of Frame::domWindow lazily created DOMWindow objects and we were careful to trigger this lazy creation at specific times.

I'm not sure what's causing the difference between release-safari and future-safari through.  I'm surprised that this doesn't happen in release-safari because it would seem to be need to balance the same logic in clearDOMWindow.

[Adam Barth]

In general, though, I agree with you that it would be better to know more, but I don't want to burden smfr more than we have already here.

[Adam Barth]

s/not explicitly/now explicitly/

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/126090