Bug 13038 – ASSERTION FAILED: item->target().isEmpty() || m_mainFrame->tree()->find(item->target()) == m_mainFrame

• Component: History
• Status: RESOLVED FIXED

[Matt Lilek]

I keep seeing this assertion failure (more frequently lately) but can't reproduce it until it pops up again.  It always happens after the browser's been open for awhile, has a few tabs open and going back in a recently opened tab seems to hit it.  I've seen it going back on Apple.com, MySpace and Amazon's "Look Inside" thing.

ASSERTION FAILED: item->target().isEmpty() || m_mainFrame->tree()->find(item->target()) == m_mainFrame
(/Users/matt/Code/WebKit/WebCore/page/Page.cpp:132 void WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType))

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xbbadbeef

Thread 0 Crashed:
0   com.apple.WebCore              	0x011f30e8 WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 312 (Page.cpp:132)
1   com.apple.WebCore              	0x011f3238 WebCore::Page::goBack() + 88 (Page.cpp:113)
2   com.apple.WebKit               	0x00385294 -[WebView goBack] + 48 (WebView.mm:1972)
3   com.apple.WebKit               	0x003885ec -[WebView(WebIBActions) goBack:] + 64 (WebView.mm:2459)
4   com.apple.AppKit               	0x937aac4c -[NSApplication sendAction:to:from:] + 108
[snip]

[Mark Rowe (bdash)]

I saw this just now.  I visited digg.com, clicked on a link about Ubuntu, clicked on a link to a screenshot, then hit back.  *kaboom*.  Digg is currently down for maintenance, hopefully this will reproduce when it comes back up.

[David Kilzer (:ddkilzer)]

Just saw this after going to http://www.openmoko.org/, then clicking on the "Projects" link on the top right side of the page.  Started filing a bug on the rounded edges misrendering on the page, then tried to go back and got the assertion:


Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xbbadbeef

Thread 0 Crashed:
0   com.apple.WebCore        	0x011e774c WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 312 (Page.cpp:132)
1   com.apple.WebCore        	0x011e789c WebCore::Page::goBack() + 88 (Page.cpp:113)
2   com.apple.WebKit         	0x003862f0 -[WebView goBack] + 80 (WebView.mm:2004)
3   com.apple.WebKit         	0x003898b0 -[WebView(WebIBActions) goBack:] + 68 (WebView.mm:2513)
4   com.apple.AppKit         	0x9383fc4c -[NSApplication sendAction:to:from:] + 108
5   com.apple.Safari         	0x0002956c 0x1000 + 165228
6   com.apple.AppKit         	0x9383fb80 -[NSControl sendAction:to:] + 96
7   com.apple.AppKit         	0x9383fa60 -[NSCell _sendActionFrom:] + 156
8   com.apple.AppKit         	0x93859a88 -[NSCell trackMouse:inRect:ofView:untilMouseUp:] + 1020
9   com.apple.AppKit         	0x93859670 -[NSButtonCell trackMouse:inRect:ofView:untilMouseUp:] + 564
10  com.apple.AppKit         	0x93859094 -[NSControl mouseDown:] + 536
11  com.apple.Safari         	0x00054614 0x1000 + 341524
12  com.apple.AppKit         	0x937fa890 -[NSWindow sendEvent:] + 4616
13  com.apple.Safari         	0x00021734 0x1000 + 132916
14  com.apple.AppKit         	0x937a38d4 -[NSApplication sendEvent:] + 4172
15  com.apple.Safari         	0x00021238 0x1000 + 131640
16  com.apple.AppKit         	0x9379ad10 -[NSApplication run] + 508
17  com.apple.AppKit         	0x9388b87c NSApplicationMain + 452
18  com.apple.Safari         	0x0005c77c 0x1000 + 374652
19  com.apple.Safari         	0x0005c624 0x1000 + 374308

Console output:

ASSERTION FAILED: item->target().isEmpty() || m_mainFrame->tree()->find(item->target()) == m_mainFrame
(/path/to/WebKit/WebCore/page/Page.cpp:132 void WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType))
Segmentation fault

Using a local debug build of WebKit r21042 with Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

See also bug 13700.

[David Kilzer (:ddkilzer)]

*** Bug 13700 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

Created attachment 14525 [details]
Test case

* STEPS TO REPRODUCE
1. Open this test case attachment in a new window.
2. Command-click on the link to open a new tab.
3. Click on the second tab to switch to it.  (I don't believe you must wait for it to finish loading.)
4. Click on the "Patch" attachment (without scrolling the page; not sure if that matters or not).
5. Hit Command-Left-Arrow to return to the previous page.
6. Crash occurs.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Created an attachment (id=14525) [edit]
> Test case

The key seems to be the target="_blank" attribute in the anchor tag.

[Maxime BRITTO]

The assert calls the FrameTree::find() method which analyses the target passed as a string parameter and return 0 if it's a "_blank" target (because _blank does not appear in a FramTree).

I think the has no reason to be when the target is a _blank so I tried to replace the old ASSERT by this :
    // Handle the go back/forward to a frameset, don't need to handle if the traget is a _blank
    // We never go back/forward on a per-frame basis, so the target must be the main frame
    ASSERT(item->target().isEmpty() || item->target().contains("_blank", true) || m_mainFrame->tree()->find(item->target()) == m_mainFrame);

It work and the bug is no longer reproductible with this.

Unfortunately I have a layout test error which seems linked to my modification : http/tests/navigation/relativeanchor-goback	expected	actual	diffs

(In reply to comment #7)

> Unfortunately I have a layout test error which seems linked to my modification
> : http/tests/navigation/relativeanchor-goback    expected        actual  diffs

A good way to tell if a test failure is caused by a local change is to check the build bot at <http://build.webkit.org>. As you can see, that test is on of two tests failing on the bot currently: <http://build.webkit.org/results/post-commit-powerpc-mac-os-x/6916/results.html>. Bug 13934 is tracking the failure.

[Maxime BRITTO]

(In reply to comment #8)
> A good way to tell if a test failure is caused by a local change is to check
> the build bot at <http://build.webkit.org>. As you can see, that test is on of
> two tests failing on the bot currently:
> <http://build.webkit.org/results/post-commit-powerpc-mac-os-x/6916/results.html>.
> Bug 13934 is tracking the failure.
> 

Great so I prepare and send the patch on monday morning (French hour) when I get back to my workstation.
Good Weekend to all of you.

[Maxime BRITTO]

Created attachment 14930 [details]
Patch proposition to avoid the assertion error

With this modification I can't reproduce the assertion error.

[Mark Rowe (bdash)]

Comment on attachment 14930 [details]
Patch proposition to avoid the assertion error

To make changes easier to follow, we prefer to separate style changes from behavioural changes.  If you could prepare a patch that only includes your behavioural change it would be much easier to review + land.

[Maxime BRITTO]

Created attachment 15016 [details]
proposed patch 2

(In reply to comment #11)
> (From update of attachment 14930 [details] [edit])
> To make changes easier to follow, we prefer to separate style changes from
> behavioural changes.  If you could prepare a patch that only includes your
> behavioural change it would be much easier to review + land.
> 
I cleaned from the text file all the modifications which weren't related to this bug, I hope that was really what you meant in your comment.

[Matt Lilek]

(In reply to comment #12)
> Created an attachment (id=15016) [edit]
> proposed patch 2
> 
> (In reply to comment #11)
> > (From update of attachment 14930 [details] [edit] [edit])
> > To make changes easier to follow, we prefer to separate style changes from
> > behavioural changes.  If you could prepare a patch that only includes your
> > behavioural change it would be much easier to review + land.
> > 
> I cleaned from the text file all the modifications which weren't related to
> this bug, I hope that was really what you meant in your comment.
> 

I'm not a reviewer, but whoever does review this patch is going to suggest you fill in your name and email address in the ChangeLog as well as at least add a link to this bug report.  An explanation of the change would be even better.  It also probably wouldn't hurt to look into making the test case attached to this bug into a layout test.  Whether those things qualify for a r-, I'm not sure.

[Brady Eidson]

Comment on attachment 15016 [details]
proposed patch 2

Please fill in your name and email address at the head of your change log (more info on the env. variables that would do it can be found at webkit.org)

Please describe the change in the changelog

The .xcodeproj change is not necessary

And finally, this *will* need a layout test.

Good exploration and code change though!  I'll r+ it once those things have been addressed

[Maxime BRITTO]

Created attachment 15049 [details]
incomplete HTML layout test

(In reply to comment #14)
> (From update of attachment 15016 [details] [edit])
> Please fill in your name and email address at the head of your change log (more
> info on the env. variables that would do it can be found at webkit.org)
> 
> Please describe the change in the changelog
> 
> The .xcodeproj change is not necessary
> 
> And finally, this *will* need a layout test.
> 
> Good exploration and code change though!  I'll r+ it once those things have
> been addressed
> 
I cleaned up my changelog, added a bief description of the modifications and filled up the blanks about my name and mail.
But, about the layout test, I've spent a lot of time trying to create one without success because I couldn't find out how to open a page in a tab (command-click) with Javascript.
I'm attaching the html file I wrote, if someone know how to modify it to make WebKit crash, feel free to do it.

[Maxime BRITTO]

Created attachment 15050 [details]
proposed patch3

Here is the cleaned up patch.

[Maxime BRITTO]

Created attachment 15052 [details]
proposed patch3bis

I sent the wrong version of the file before. Here is the good one.

[Maxime BRITTO]

Created attachment 15180 [details]
patch with layout test

After asking on the IRC channel, I've been suggested to prepare a manual test case with everything clearly described. The test can be run by the layout test controller but it's really effective when reproduced manually.

[Maxime BRITTO]

Created attachment 15221 [details]
proposed patch

Tabs replaced by spaces.

[Maciej Stachowiak]

Comment on attachment 15221 [details]
proposed patch

r=me

[Mark Rowe (bdash)]

It'd be great if the test case could be cleaned up a bit before it was committed.  The formatting of the text in some of the pages makes no sense -- it's laid out as multiple paragraphs within the source but with no HTML markup to indicate it as such, so it all gets run into a single paragraph when displayed by the browser.  The use of punctuation within some of the sentences is rather bizarre, and there are several spelling errors.

+if (window.layoutTestController) {
+    layoutTestController.dumpAsText();
+    alert("This should be a manual test");

It's unclear why there is an alert here claiming that this should be a manual test.

[Maxime BRITTO]

Created attachment 15244 [details]
patch with cleaned up test case

I cleaned up the test case as Mark Rowe asked.
This test case and the test case for the patch of Bug 14147 (r?) are using the same file (will-go-back.html). Since I don't know which patch will be committed first, the file is in the both patchs.

[Geoffrey Garen]

Comment on attachment 15244 [details]
patch with cleaned up test case

Wow! This patch has a long history. I believe all comments above have been addressed.

Whoever lands this patch: please add newlines to the ends of the files. I would ask the poster to do that, but I don't want to delay this patch any longer over style issues.

[Sam Weinig]

Landed with cleanup in r23861.

[Sam Weinig]

I had to back out this patch as it caused a the intel buildbot to fail many layout tests.  Reopening. 

[Geoffrey Garen]

I think these lines may be redundant, and the cause of the failure:

+	layoutTestController.queueScript("performUserEvents()");
+	layoutTestController.queueLoad("resources/before-go-back.html");
+	layoutTestController.queueLoad("resources/will-go-back.html");
+	layoutTestController.queueBackNavigation(1);

[Sam Weinig]

Comment on attachment 15244 [details]
patch with cleaned up test case

I am r-ing this patch as I tried the layout tests without the change to WebCore and could not reproduce the intended assertion.

[Maxime BRITTO]

Created attachment 15314 [details]
another patch :)

There were some rests from my attemps to reproduce the cmd-click (to open in a tab). I thinks that's why the bot failed this test.
About the comment #27 : did you do the test manually or with the DumpRenderTree ? Because this test must be donne manually, else it's useless. Indeed the first link must be opened in a new tab which I couldn't reproduce with th LayoutTestController.

[Maciej Stachowiak]

Comment on attachment 15314 [details]
another patch :)

If the test has to be done manually, then instead of LayoutTests it should go in WebCore/manual-tests.

Otherwise patch looks fine. I will mark this r- but if you can get a committer to fix this while landing that's cool.

[Maxime BRITTO]

Created attachment 15395 [details]
patch version h

I moved the files at the right place.Hope this one will be the one...

[Maciej Stachowiak]

Comment on attachment 15395 [details]
patch version h

r=me

[Mark Rowe (bdash)]

If it's a manual test, why does it have the "if (window.layoutTestController) { ... }" block?

[Mark Rowe (bdash)]

Landed in r24057.