Bug 21855 – REGRESSION (r37323): Gmail complains about popup blocking when opening a link

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alexey Proskuryakov]

When clicking on a link in an e-mail, Gmail displays a pop-up saying "Grrr! A popup blocker may be preventing Gmail from opening the page.<...>" However, the popup opens successfully after that.

The message also appears when clicking on web clip links.

This is a regression from Safari 3.1.2.

<rdar://problem/6278244>

[Feng Qian]

I cannot reproduce the first issue described in webkit nightly build 37990 on both Mac and Windows.  

[Alexey Proskuryakov]

That's strange, I can reproduce the problem with r37990 nightly on a clean Mac OS X 10.5.5 system when clicking on a bugzilla link in a WebKit commit e-mail. Using "newer version" Gmail with English (US) localization.

Probably a stupid question, but do you have popup blocker enabled? Also, you are saying that you could not reproduce the first issue - what about the second one (web clip)?

[Feng Qian]

Sorry for the confusion, I didn't enable pop-up blocker.

Yes, I can reproduce it on Mac with latest build, but I have no clue what's happening yet.

(In reply to comment #2)
> That's strange, I can reproduce the problem with r37990 nightly on a clean Mac
> OS X 10.5.5 system when clicking on a bugzilla link in a WebKit commit e-mail.
> Using "newer version" Gmail with English (US) localization.
> 
> Probably a stupid question, but do you have popup blocker enabled? Also, you
> are saying that you could not reproduce the first issue - what about the second
> one (web clip)?
> 

[Cameron Zwarich (cpst)]

This regressed in r37323:

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

[Cameron Zwarich (cpst)]

The problem is that JSAbstractEventListener::handleEvent() never sets the dynamicGlobalObject of the JSGlobalData before entering script execution.

[Cameron Zwarich (cpst)]

Created attachment 26089 [details]
Patch in progress

This patch fixes the bug. It passes all tests, but I need to make a new test for this bug.

[Alexey Proskuryakov]

Is the same fix necessary wherever WebCore uses JSC::call()?

[Cameron Zwarich (cpst)]

(In reply to comment #7)
> Is the same fix necessary wherever WebCore uses JSC::call()?

It is necessary whenever the function called may have originated with a different global object and the goal is to execute it within the context of the current global object. I imagine that is most cases where WebCore uses JSC::call(). Since so few things depend on the dynamic global object at the moment, it might be hard to find other real-world cases where there is incorrect behaviour.

[Cameron Zwarich (cpst)]

Geoff pointed out to me that this shouldn't be special cased for event handlers, since they can fire synchronously from JavaScript.

[Cameron Zwarich (cpst)]

Created attachment 26118 [details]
Proposed patch

[Geoffrey Garen]

Comment on attachment 26118 [details]
Proposed patch

> +        function test()
> +        {
> +            if (window.layoutTestController) {
> +                eventSender.mouseMoveTo(2, 2);
> +                eventSender.scheduleAsynchronousClick();
> +                // eventSender.mouseDown();
> +                // eventSender.mouseUp();
> +            }
> +        }

Please remove the commented-out code.

For future reference, eventSender has a "leapForward" method that allows you to pretend that time has passed between events. I'm not sure if that would have been useful to you here or not.

Another way to answer Alexey's question: We don't know of any cases other than event handling where the dynamic global object should be something other than the lexical global object. But maybe there are some.

r=me

[Cameron Zwarich (cpst)]

(In reply to comment #11)
> (From update of attachment 26118 [details] [review])
> > +        function test()
> > +        {
> > +            if (window.layoutTestController) {
> > +                eventSender.mouseMoveTo(2, 2);
> > +                eventSender.scheduleAsynchronousClick();
> > +                // eventSender.mouseDown();
> > +                // eventSender.mouseUp();
> > +            }
> > +        }
> 
> Please remove the commented-out code.

Oops. My bad. I will fix that.

> For future reference, eventSender has a "leapForward" method that allows you to
> pretend that time has passed between events. I'm not sure if that would have
> been useful to you here or not.

It doesn't help in my case (I tried), because all of the events between the mouseDown and mouseUp get queued and delivered at once upon the mouseUp. However, there is still the JS on the C stack that triggered the mouseUp.

[Cameron Zwarich (cpst)]

Fixed in r39377.