Bug 13038 - ASSERTION FAILED: item->target().isEmpty() || m_mainFrame->tree()->find(item->target()) == m_mainFrame
Summary: ASSERTION FAILED: item->target().isEmpty() || m_mainFrame->tree()->find(item-...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
: 13700 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-03-10 15:15 PST by Matt Lilek
Modified: 2007-07-06 03:33 PDT (History)
6 users (show)

See Also:


Attachments
Test case (120 bytes, text/html)
2007-05-12 16:19 PDT, David Kilzer (:ddkilzer)
no flags Details
Patch proposition to avoid the assertion error (6.03 KB, patch)
2007-06-11 01:34 PDT, Maxime BRITTO
mrowe: review-
Details | Formatted Diff | Diff
proposed patch 2 (1.87 KB, patch)
2007-06-14 01:14 PDT, Maxime BRITTO
beidson: review-
Details | Formatted Diff | Diff
incomplete HTML layout test (463 bytes, text/html)
2007-06-15 05:31 PDT, Maxime BRITTO
no flags Details
proposed patch3 (1.36 KB, patch)
2007-06-15 05:47 PDT, Maxime BRITTO
mbritto: review-
Details | Formatted Diff | Diff
proposed patch3bis (1.47 KB, patch)
2007-06-15 06:14 PDT, Maxime BRITTO
no flags Details | Formatted Diff | Diff
patch with layout test (6.39 KB, patch)
2007-06-22 05:23 PDT, Maxime BRITTO
no flags Details | Formatted Diff | Diff
proposed patch (6.57 KB, patch)
2007-06-25 07:56 PDT, Maxime BRITTO
sam: review-
Details | Formatted Diff | Diff
patch with cleaned up test case (6.54 KB, patch)
2007-06-26 03:48 PDT, Maxime BRITTO
sam: review-
Details | Formatted Diff | Diff
another patch :) (5.82 KB, patch)
2007-06-29 07:15 PDT, Maxime BRITTO
mjs: review-
Details | Formatted Diff | Diff
patch version h (4.12 KB, patch)
2007-07-05 01:20 PDT, Maxime BRITTO
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lilek 2007-03-10 15:15:49 PST
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]
Comment 1 Mark Rowe (bdash) 2007-03-14 22:39:48 PDT
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.
Comment 2 David Kilzer (:ddkilzer) 2007-04-23 14:37:06 PDT
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).

Comment 3 mitz 2007-05-12 16:08:16 PDT
See also bug 13700.
Comment 4 David Kilzer (:ddkilzer) 2007-05-12 16:14:10 PDT
*** Bug 13700 has been marked as a duplicate of this bug. ***
Comment 5 David Kilzer (:ddkilzer) 2007-05-12 16:19:52 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2007-05-12 16:25:35 PDT
(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.

Comment 7 Maxime BRITTO 2007-06-08 07:37:25 PDT
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
Comment 8 mitz 2007-06-08 07:54:51 PDT
(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.
Comment 9 Maxime BRITTO 2007-06-09 03:11:22 PDT
(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.
Comment 10 Maxime BRITTO 2007-06-11 01:34:40 PDT
Created attachment 14930 [details]
Patch proposition to avoid the assertion error

With this modification I can't reproduce the assertion error.
Comment 11 Mark Rowe (bdash) 2007-06-13 13:57:05 PDT
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.
Comment 12 Maxime BRITTO 2007-06-14 01:14:04 PDT
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.
Comment 13 Matt Lilek 2007-06-14 10:16:29 PDT
(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.
Comment 14 Brady Eidson 2007-06-14 18:08:24 PDT
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
Comment 15 Maxime BRITTO 2007-06-15 05:31:06 PDT
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.
Comment 16 Maxime BRITTO 2007-06-15 05:47:39 PDT
Created attachment 15050 [details]
proposed patch3

Here is the cleaned up patch.
Comment 17 Maxime BRITTO 2007-06-15 06:14:45 PDT
Created attachment 15052 [details]
proposed patch3bis

I sent the wrong version of the file before. Here is the good one.
Comment 18 Maxime BRITTO 2007-06-22 05:23:23 PDT
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.
Comment 19 Maxime BRITTO 2007-06-25 07:56:29 PDT
Created attachment 15221 [details]
proposed patch

Tabs replaced by spaces.
Comment 20 Maciej Stachowiak 2007-06-25 19:44:44 PDT
Comment on attachment 15221 [details]
proposed patch

r=me
Comment 21 Mark Rowe (bdash) 2007-06-25 20:07:11 PDT
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.
Comment 22 Maxime BRITTO 2007-06-26 03:48:50 PDT
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.
Comment 23 Geoffrey Garen 2007-06-26 14:00:17 PDT
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.
Comment 24 Sam Weinig 2007-06-28 14:09:54 PDT
Landed with cleanup in r23861.
Comment 25 Sam Weinig 2007-06-28 16:04:34 PDT
I had to back out this patch as it caused a the intel buildbot to fail many layout tests.  Reopening. 
Comment 26 Geoffrey Garen 2007-06-28 16:31:35 PDT
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);
Comment 27 Sam Weinig 2007-06-28 17:14:09 PDT
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.
Comment 28 Maxime BRITTO 2007-06-29 07:15:26 PDT
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.
Comment 29 Maciej Stachowiak 2007-07-04 03:36:29 PDT
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.
Comment 30 Maxime BRITTO 2007-07-05 01:20:20 PDT
Created attachment 15395 [details]
patch version h

I moved the files at the right place.Hope this one will be the one...
Comment 31 Maciej Stachowiak 2007-07-05 19:11:40 PDT
Comment on attachment 15395 [details]
patch version h

r=me
Comment 32 Mark Rowe (bdash) 2007-07-06 03:24:06 PDT
If it's a manual test, why does it have the "if (window.layoutTestController) { ... }" block?

Comment 33 Mark Rowe (bdash) 2007-07-06 03:33:09 PDT
Landed in r24057.