Bug 12117 – REGRESSION: Crash when clicking on a link in gmail

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

[Benjamin Kuperman]

Using the latest nightly build (18554) I've found that certain links will crash when they are followed from a message in gmail.  However, the link itself does not seem to cause the crash when manually entered or followed from a HTML page.

To reproduce, mail yourself the following link @gmail and then view the message and click on the link.  You should see a new window pop up, it looks like some redirection takes place, and then it crashes.

http://www.e-retention.com/ct/ct.php?t=1101294&c=1014909640&m=m&type=1&h=E27AA4CE0A0E22303EE56531B55FECD7

I have no idea if this is related to bug#11977, but that write up does not mention clicking on a link.

[Benjamin Kuperman]

Created attachment 12226 [details]
Crash log

[David Kilzer (:ddkilzer)]

This may be a duplicate of Bug 12087.  Please retest a nightly WebKit build of r18588 or newer when it becomes available.

[Benjamin Kuperman]

Created attachment 12234 [details]
Crash log #2

(In reply to comment #2)
> This may be a duplicate of Bug 12087.  Please retest a nightly WebKit build of
> r18588 or newer when it becomes available.

Sorry, no joy with r18602.  Still get a crash.  See attached.

[David Kilzer (:ddkilzer)]

Confirmed with locally-built debug build of WebKit r18609 with Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8N1037).

[David Kilzer (:ddkilzer)]

Stack trace from debug build:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000008

Thread 0 Crashed:
0   com.apple.WebCore        	0x0148c915 WTF::RefPtr<WebCore::StringImpl>::get() const + 9 (RefPtr.h:45)
1   com.apple.WebCore        	0x0148c92d WebCore::String::impl() const + 17 (PlatformString.h:150)
2   com.apple.WebCore        	0x014d7acf WebCore::operator!=(WebCore::String const&, WebCore::String const&) + 31 (PlatformString.h:201)
3   com.apple.WebCore        	0x013b57fd WebCore::HistoryItem::setURLString(WebCore::String const&) + 33 (HistoryItem.cpp:215)
4   com.apple.WebCore        	0x013b6221 WebCore::HistoryItem::setURL(WebCore::KURL const&) + 63 (HistoryItem.cpp:226)
5   com.apple.WebCore        	0x013987af WebCore::FrameLoader::updateHistoryForStandardLoad() + 407 (FrameLoader.cpp:2990)
6   com.apple.WebCore        	0x0139b9bd WebCore::FrameLoader::transitionToCommitted(WTF::PassRefPtr<WebCore::PageCache>) + 607 (FrameLoader.cpp:2024)
7   com.apple.WebCore        	0x0139bef1 WebCore::FrameLoader::commitProvisionalLoad(WTF::PassRefPtr<WebCore::PageCache>) + 191 (FrameLoader.cpp:1933)
8   com.apple.WebCore        	0x01370bdf WebCore::DocumentLoader::commitIfReady() + 93 (DocumentLoaderMac.mm:301)
9   com.apple.WebCore        	0x01370c0f WebCore::DocumentLoader::commitLoad(char const*, int) + 35 (DocumentLoaderMac.mm:340)
10  com.apple.WebCore        	0x01370e52 WebCore::DocumentLoader::receivedData(char const*, int) + 76 (DocumentLoaderMac.mm:354)
11  com.apple.WebCore        	0x0137221b WebCore::FrameLoader::receivedData(char const*, int) + 41 (FrameLoaderMac.mm:511)
12  com.apple.WebCore        	0x01379540 WebCore::MainResourceLoader::addData(char const*, int, bool) + 80 (MainResourceLoaderMac.mm:145)
13  com.apple.WebCore        	0x01377f87 WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) + 83 (ResourceLoaderMac.mm:273)
14  com.apple.WebCore        	0x013799dd WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) + 281 (MainResourceLoaderMac.mm:297)
15  com.apple.WebCore        	0x01377b74 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) + 58 (ResourceLoaderMac.mm:433)
16  com.apple.WebCore        	0x0138678a -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] + 172 (ResourceHandleMac.mm:291)
17  com.apple.Foundation     	0x9265eb86 -[NSURLConnection(NSURLConnectionInternal) _sendDidReceiveDataCallback] + 641
18  com.apple.Foundation     	0x9265ce67 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 686
19  com.apple.Foundation     	0x9265cb41 _sendCallbacks + 201
20  com.apple.CoreFoundation 	0x9082afd2 CFRunLoopRunSpecific + 1213
21  com.apple.CoreFoundation 	0x9082ab0e CFRunLoopRunInMode + 61
22  com.apple.HIToolbox      	0x92ddabef RunCurrentEventLoopInMode + 285
23  com.apple.HIToolbox      	0x92dda2fd ReceiveNextEventCommon + 385
24  com.apple.HIToolbox      	0x92dda154 BlockUntilNextEventMatchingListInMode + 81
25  com.apple.AppKit         	0x9327f465 _DPSNextEvent + 572
26  com.apple.AppKit         	0x9327f056 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 137
27  com.apple.Safari         	0x00006cea 0x1000 + 23786
28  com.apple.AppKit         	0x93278ddb -[NSApplication run] + 512
29  com.apple.AppKit         	0x9326cd2f NSApplicationMain + 573
30  com.apple.Safari         	0x0005f54a 0x1000 + 386378
31  com.apple.Safari         	0x0005f471 0x1000 + 386161

[Brady Eidson]

Crashed with same backtrace as ddkilzer
Problem is a null HistoryItem deref in frame 5 - 
WebCore::FrameLoader::updateHistoryForStandardLoad at FrameLoader.cpp:2987

Fix is coming

[Brady Eidson]

I explored a bit to make sure this wasn't something more interesting than it really turned out to be:
FNCL
(Free Nil Checking Lost)

Patch is coming

[Brady Eidson]

nope, this *is* more complicated.  Have some more exploring to do.

[Brady Eidson]

Adding a null check in the right place caused another, later null-deref.  Adding a null check there fixed it up enough to not crash.  Alas at that point, you're on a page without a "currentHistoryItem" which is supposed to be an impossibility, and things get inconsistent as you build up a back/forward list and naviagte around, and you can cause *yet another* crash.

So, blind null checking isn't the answer.  There *should* be a m_currentHistoryItem here but there's not.  Why not?
Well one interesting thing I discovered about this case is that the initial load in the new window popped up by google is a redirect...  I don't have enough energy left tonite to keep exploring this, but I suspect that the provisionalHistoryItem isn't getting created on this load for some reason or it isn't getting assigned to the currentHistoryItem correctly.

I suspect the former.  I haven't dissected exactly how gmail is doing this popup but perhaps the load gets started in the original frame and passed off to the popup, which doesn't get a provisionalHistoryItem...  or something...
I might be blowing smoke also, cause it's very late, and I'm very tired, and I'm heading home  :)

3 karma points for whoever figures this out while I sleep  ;)

[Benjamin Kuperman]

I poked around to see if I could discover the source of the problem, and it has to do with the JavaScript that is associate with the link.  Here is what I get from the Inspector:

onclick="return top.js.OpenExtLink(window,event,this)"
href="http://webkit.org/"
target="_blank"

I've determined that if I turn off JavaScript before clicking the link, everything works fine.  So the problem likely stems from actions started by the onclick event.  I've tried to get Drosera to give me the source for that function, but I obviously have much to learn about that tool.

[Brady Eidson]

I'm stepping through this code side-by-side with the old BF cache and it turns out this *is* a "missing null check" situation, but something else after I add the null checks is still causing issues.

I think working side by side with the old impl will give me the answer, stay tuned...

[Brady Eidson]

This is frustrating!  Apparently, opening links from gmail always had some bugs re: the back/forward history.
I just traced the new impl side-by-side with the old impl.  The behavior is actually no different.
The bizarre thing is that after you open the link into a new window, then navigate to a 2nd page in that new window, the first page is not added to the back/forward list because the nav to the new window never created a history item.
If you then go to a 3rd page, then click back to the 2nd page, you will start hitting assertions (in a debug build)
The funny thing is - this behavior isn't my new impl's fault, it also existed in the old impl.  It has been around since shipping Safari on Tiger

Now in a release build of the old impl - no assertions - things gracefully worked anyway, probably due to "free nil checking" in objective-c.  I fear the new impl will no longer have that net and will crash.

I'm going to submit a patch to fix up this *crash* then file a new bug on this newly discovered issue, and explore it a bit.

[Brady Eidson]

Created attachment 12247 [details]
Add some null checks

[Darin Adler]

Comment on attachment 12247 [details]
Add some null checks

Is the less-nested structure for the first if statement more logical?

r=me

[Brady Eidson]

Committed in r18626

[Brady Eidson]

Didn't have to file the new bug, it's long standing (since this misbehavior was in Tiger, even)
http://bugs.webkit.org/show_bug.cgi?id=3546