Bug 21855 - REGRESSION (r37323): Gmail complains about popup blocking when opening a link
Summary: REGRESSION (r37323): Gmail complains about popup blocking when opening a link
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords: GoogleBug, InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2008-10-24 09:46 PDT by Alexey Proskuryakov
Modified: 2008-12-18 10:19 PST (History)
4 users (show)

See Also:


Attachments
Patch in progress (841 bytes, patch)
2008-12-17 02:11 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (14.50 KB, patch)
2008-12-18 00:31 PST, Cameron Zwarich (cpst)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2008-10-24 09:46:13 PDT
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>
Comment 1 Feng Qian 2008-10-30 11:37:22 PDT
I cannot reproduce the first issue described in webkit nightly build 37990 on both Mac and Windows.  
Comment 2 Alexey Proskuryakov 2008-10-31 03:33:11 PDT
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)?
Comment 3 Feng Qian 2008-11-03 08:44:02 PST
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)?
> 
Comment 4 Cameron Zwarich (cpst) 2008-12-17 00:08:24 PST
This regressed in r37323:

http://trac.webkit.org/changeset/37323
Comment 5 Cameron Zwarich (cpst) 2008-12-17 01:12:10 PST
The problem is that JSAbstractEventListener::handleEvent() never sets the dynamicGlobalObject of the JSGlobalData before entering script execution.
Comment 6 Cameron Zwarich (cpst) 2008-12-17 02:11:41 PST
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.
Comment 7 Alexey Proskuryakov 2008-12-17 02:54:12 PST
Is the same fix necessary wherever WebCore uses JSC::call()?
Comment 8 Cameron Zwarich (cpst) 2008-12-17 03:33:24 PST
(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.
Comment 9 Cameron Zwarich (cpst) 2008-12-17 15:50:35 PST
Geoff pointed out to me that this shouldn't be special cased for event handlers, since they can fire synchronously from JavaScript.
Comment 10 Cameron Zwarich (cpst) 2008-12-18 00:31:28 PST
Created attachment 26118 [details]
Proposed patch
Comment 11 Geoffrey Garen 2008-12-18 09:44:05 PST
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
Comment 12 Cameron Zwarich (cpst) 2008-12-18 10:15:15 PST
(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.
Comment 13 Cameron Zwarich (cpst) 2008-12-18 10:19:33 PST
Fixed in r39377.