Bug 67234 – REGRESSION(r93902): Can't open external links on gmail

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Reproduction steps:
1. Go to gmail.com
2. Open some message that contains an external hyperlink
3. Open it while pressing the command key

Expected result:
The destination is opened in new tab

Actual result:
I get an alert with message "Grrr! A popup blocker may be preventing the application from opening the page. If you have a popup blocker, try disabling it to open the window."

[Ryosuke Niwa]

Oops, this bug doesn't reproduce on released version of Safari 5.1.  Only on ToT.

[Ryosuke Niwa]

r92990: good
r93707: good
r93849: good
r93903: bad
r93948: bad

[Alexey Proskuryakov]

Further bisected to r93900-r93903, meaning that it's <http://trac.webkit.org/changeset/93902> (bug 66823).

[Alexey Proskuryakov]

My steps to reproduce:
1. Open a webkit-changes list message in GMail.
2. Click or Cmd-click on revision number.

Expected results: changeset opens in a new window or tab.
Actual results: a "Grrr!" alert.

[Alexey Proskuryakov]

<rdar://problem/10053636>

[Ryosuke Niwa]

This might be also related to nightly not auto-installing new binaries.

[Alice Liu]

looking now.

[Alice Liu]

the window isn't created because sendSync returns false.  Looking further, sendSync does send the message okay, but it's having trouble decoding the reply. 

CoreIPC::ArgumentDecoder::markInvalid() at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:48
CoreIPC::ArgumentDecoder::alignBufferPosition(unsigned int, unsigned long) ()
CoreIPC::ArgumentDecoder::decodeUInt64 at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:156
CoreIPC::ArgumentDecoder::decode<unsigned long long> at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:136
CoreIPC::Arguments1<unsigned long long&>::decode at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/Arguments.h:77
CoreIPC::Arguments2<unsigned long long&, WebKit::WebPageCreationParameters&>::decode at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/Arguments.h:115
CoreIPC::ArgumentCoder<CoreIPC::Arguments2<unsigned long long&, WebKit::WebPageCreationParameters&> >::decode at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:44
CoreIPC::ArgumentDecoder::decode<CoreIPC::Arguments2<unsigned long long&, WebKit::WebPageCreationParameters&> > at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:89
CoreIPC::Connection::sendSync<Messages::WebPageProxy::CreateNewPage> at /Volumes/Data/Source/OpenSource/Source/WebKit2/Platform/CoreIPC/Connection.h:386
WebKit::WebChromeClient::createWindow at /Volumes/Data/Source/OpenSource/Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:167

[Anders Carlsson]

Created attachment 105828 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 105828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105828&action=review

This bug couldn't happen on Windows, could it?

> Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:521
> +    // FIXME: Move this to ArgumentCodersCFMac.mm and change this file back to be C++

Is there a reason why this cannot be done right away? Leaving ArgumentCodersCF.cpp in Objective C++ mode seems unfortunate.

> Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:525
> +        result = reinterpret_cast<CFURLRef>([NSURL URLWithString:@""]);

Would be nice to avoid leaving an autoreleased object behind - NSURL has initWithString:.

[Anders Carlsson]

(In reply to comment #10)
> (From update of attachment 105828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105828&action=review
> 
> This bug couldn't happen on Windows, could it?
> 

Not sure...

> > Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:521
> > +    // FIXME: Move this to ArgumentCodersCFMac.mm and change this file back to be C++
> 
> Is there a reason why this cannot be done right away? Leaving ArgumentCodersCF.cpp in Objective C++ mode seems unfortunate.

I wanted to get this fix landed as soon as possible.

> 
> > Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:525
> > +        result = reinterpret_cast<CFURLRef>([NSURL URLWithString:@""]);
> 
> Would be nice to avoid leaving an autoreleased object behind - NSURL has initWithString:.

I couldn't find an elegant way to do that given that we want to call adoptNS on a RetainPtr<CFURLRef>.

[Anders Carlsson]

Committed r94246: <http://trac.webkit.org/changeset/94246>

[Alexey Proskuryakov]

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

[Adam Roben (:aroben)]

Is it possible to write a test for this?