Closed Bug 252326 Opened 20 years ago Closed 20 years ago

aol.com opens popup window by appending a script element

Categories

(SeaMonkey :: UI Design, defect, P2)

Product:
SeaMonkey
Component:
UI Design
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: danm.moz, Assigned: ecfbugzilla)

References

(
URL
)

Details

(Keywords: fixed-aviary1.0, fixed1.7.5, Whiteboard: [sg:nse,fix] longtest)

Attachments

(9 files, 19 obsolete files)

separate script file for testcase
(deleted), text/javascript
Details
testcase
(deleted), text/html
Details
shows both onload and setTimeout
(deleted), text/html
Details
Part 5: Remove CurrentEventShepherd from EventStateManager
(deleted), patch
Details | Diff | Splinter Review
157740: Part 4: The actual popup blocker patch
(deleted), patch
Details | Diff | Splinter Review
Address reviewer comments.
(deleted), patch
Details | Diff | Splinter Review
Address reviewer comments (trunk version).
(deleted), patch
brendan
: superreview+
Details | Diff | Splinter Review
Final aviary diff for checkin (includes post checkin fixes from the trunk)
(deleted), patch
jst
: review+
jst
: review+
jst
: superreview+
jst
: superreview+
dveditz
: approval1.7.5+
Details | Diff | Splinter Review
mochitest compatible testcase
(deleted), application/xhtml+xml
jst
: review+
Details
www.aol.com has discovered an exploit that consistently circumvents Mozilla's popup blocker. It works by appending a DOM script element and starting another load to populate it. It's executed after the page thinks it has finished loading. Something like so: <body onload="openPopupWindow()"> <script> function openPopupWindow() { var sObj = document.getElementsByTagName("head")[0] .appendChild(document.createElement("script")); sObj.src = "openWindow.js"; } </script> It works only if called from the load handler (if simply executed as naked script, it executes early enough for the document to still consider itself unloaded). And the script source must be loaded from an external file.
Attached file separate script file for testcase (deleted) —
Attached file testcase (deleted) —
Flags: blocking-aviary1.0RC1+
Flags: blocking-aviary1.0+
Priority: -- → P2
Assignee: nobody → jst
IE6 on WinXP servicepack 2 manages to block this. Asa also noticed that Opera (version?) on WinXP also blocks the popup.
...although, interestingly: when I turn off IE's popup blocking, the new window doesn't display (the main browser window loses focus for a bit then regains it). so IE still has bug[s] actually allowing .js to open that new window.
Whiteboard: [sg:nse,fix]
Well, Safari doesn't seem to deal with the JS correctly, so I don't know if it would actually block the popup or not.
Interesting, if they did this just for us. So when .src is set, does that cause an asynchronous load of the script and that's how it by-passes our popup blocking mechanism? If it gets lucky and the script load takes long enough for onload to finish and the document to consider itself loaded. Would this same trick work from a setTimeout too then? I thought that at some point we started using a whitelist for which contexts we allow window.open from, instead of trying to blacklist it. I'm probably wrong though, and too tired right now to bother reading the source.
Yep, this works equally well from a setTimeout.
Who needs setTimeout anyway. It works directly from onclick.
Except I guess that doesn't matter really, since we actually allow opening new windows from (user) clicks... So nevermind that last comment :-)
Attached file shows both onload and setTimeout (deleted) —
So how can we fix this? Is there a way to find out what context (onload, setTimeout, ?, ...) a script load was kicked off from? If not, should we make that information available? Or can we change the checks to say "only allow popups from user click, ?, ..."? Would that be too risky in that we'll break unexpected/unknown allowed contexts?
I think this bug is probably best addressed by re-clearing the mIsDocumentLoaded flag as the tardy script is loaded. Johnny is probably an appropriate owner for this bug.
Blocks: pop-up-arms-race
Clearing confidentiality flag, since evading the popup blocker is not a major security issue, and methods for such evasion are publically known already anyway.
Group: security
This trick (as well as bug 187255 and bug 253780) works because it runs the script after the event handler is finished so there is no current event. Right now we always allow to open windows when there is no current event, the only exception is a timeout handler. I think the proper solution to the problem would generally forbid opening windows instead, making <a href="javascript:openNewWindow()"> the only exception (at least this is the only exception I found). This one exception case shouldn't be too difficult to trace (we have to separate it from location.href='javascript:openNewWindow()' though).
It seems that it is enough to set a special flag before InternalLoad() is called in nsWebShell::OnLinkClickSync() and clear it after the call. Then we can block any attempts to open a window without a current event unless this flag is set - any user-initiated actions (clicking a link or an image map, submitting a form etc) go through nsWebShell::OnLinkClickSync(), script-initiated actions don't. Is this a possibility or am I missing something?
Your analysis in comment 14 is mostly correct though a little off in some of the details. There are several classes of processing for which no event is available, including the load event, which has never been used directly by the popup blocker. Yes a system to determine whether an attempt to open a window was triggered by a user action would solve many if not all our problems. Think of bug 101190, for starters. It's pretty clear we need to go that route. However a global flag wouldn't be my preferred method of implementing such a thing. It's worth considering, though.
(In reply to comment #16) > There are several classes of processing for which no event is > available, including the load event But they still shouldn't be able to open windows? Or are there any other cases where we have to allow it? > However a global flag > wouldn't be my preferred method of implementing such a thing. I was thinking about a private window field like mRunningTimeout - is that what you mean by "a global flag"? The only alternative would be to push a parameter all the way to the popup blocker but I don't think this is doable (it's quite a way to go). This comment doesn't really seem to object, I'm starting to write a patch then.
Attached patch Proposed patch (obsolete) (deleted) — — Splinter Review
I've tested this patch with testcases from this bug, bug 187255 and bug 253780, as well as some other valid and invalid approaches to open a window - all of them are working correctly. But I might have forgotten something - is there a test suite for the popup blocker somewhere?
Assignee: jst → trev
Status: NEW → ASSIGNED
Attachment #155283 - Flags: review?(danm.moz)
After clearing cookies and visiting http://www.aol.com, the page will be displayed for a second, and then the page goes blank and the throbber will throb indefinitely while the status bar says "Transferring data from http300.content.ru4.com", never timing out. At this point www.aol.com sets two cookies, "nonmember_onload" and "prompted", and if you try to go to http://www.aol.com again, the page will load without a problem (caveat: if you click "Reload" during the indefinite page load, the problem recurs - you have to use the address bar to get to http://www.aol.com the second time to get it to work). As long as those cookies exist, there is no problem. Erase them and revisit http://www.aol.com to recreate the problem. I have duplicated this problem on Mozilla 1.4 through Mozilla 1.7.2, and the latest Mozilla nightly from the trunk, as well as in Firefox 0.9.3, on a dozen PCs at work and at home, and using different ISPs (all under Windows 2000). One of the scripts referred to on http://www.aol.com is http://cdn.aol.com/js/wr5_aolcom_popup1.js, which includes the snippet of script mentioned in Comment #1. If I prevent wr5_aolcom_popup1.js from running when I visit http://www.aol.com, the page seems to load fine (i.e., no infinite loading on a blank page like before). This problem started on or about July 23 (which corresponds nicely with the creation of this ticket). I would open this as an evangelism ticket, but if this (Bug# 252326) is about to be resolved, I imagine it would only invalidate the evangelism ticket. Or should I open it anyway?
Wladimir: Thanks for the patch! I do intend to take a look soon. This new tack does need to be looked at carefully. Unfortunately I know of no good, thorough suite for testing window.open. It's a matter of getting everybody to try everything they can think of, as you say. rcummins: This doesn't sound like an evangelism issue. Sounds like networking troubles. You describe what sounds like Mozilla being perhaps too forgiving while waiting for the last script, hosted on a third-party server, of www.aol.com to load. By the way I don't see any of the troubles you describe on my machine. Are all your machines on a single network? Regardless as I can tell you're aware, these things are unrelated to this bug. Feel free to open another bug if you like. If it can no longer be reproduced after this bug is closed, that won't be the first time a bug has evaporated without really being fixed. Maybe you can reproduce it with a local testcase that wouldn't be dependent on this bug being fixed. As I've said, it may depend on some quality of your network. What AOL is doing here is temporarily hijacking your browser; exploiting a weakness they've found to force it to do something the user has explicitly asked it not to do, in spite of its attempts to do as the user requests. I think that's a useful subject for an evangelism bug.
Flags: blocking-aviary1.0PR+ → blocking-aviary1.0PR-
Blocks: 219064
Blocks: popups
The patch won't work for something simple like: <a href="javascript:parent.open('whatever')"> (in a frame). With this patch, such a call will be blocked. Of course the same problem plagues the existing currentEvent stuff -- an onmouseover in a subframe that calls parent.open() can open a window... Testcase: data:text/html,<frameset rows="*"><frame src="data:text/html,<div onmouseover='parent.open()'>aaa</div>"></frameset>
One other thing. The docshell code changed by that patch is used by form submission, which can be scripted. So I suspect that patch allows people to use: <form target="_blank" action="popup" name="evil"> <script> document.evil.submit(); </script> to their heart's content.
No longer blocks: popups
Comment on attachment 155283 [details] [diff] [review] Proposed patch This one isn't quite going to cut it; see comments above. We've been discussing this. Consensus is that we need something like this patch but with a true static global to catch the parent.open() edge case. Wladimir, resubmit with that minor change and I believe we'd take the patch. Some additional work needs to be done as well to catch scripted chicanery. I'm looking at that right now.
Attachment #155283 - Flags: review?(danm.moz) → review-
Attached patch Patch v2 (obsolete) (deleted) — — Splinter Review
This is the same thing but with a global flag, it solves the problem with <a href="javascript:parent.open()">. The solution for the form submission is more difficult. The only way I see is adding a parameter to nsILinkHandler::OnLinkClickSync() - whether or not it is called by an user-initiated event. This parameter will be always PR_TRUE for any caller but nsFormSubmission. nsHTMLFormElement will have to evaluate the event - if it has a submit event (not a submit() call) and the event is trusted (not a dispatchEvent() call), it will forward PR_TRUE to nsFormSubmission, otherwise PR_FALSE. Any objections or better suggestions?
Attachment #155283 - Attachment is obsolete: true
Attached patch nsIEventStateManager::GetCurrentEvent() fix (obsolete) (deleted) — — Splinter Review
This is the solution for the other problem Boris brought up: data:text/html,<frameset rows="*"><frame src="data:text/html,<div onmouseover='parent.open()' onclick='parent.open()'>aaa</div>"></frameset> Currently it opens popups for both mouseover and click events because nsIEventStateManager::GetCurrentEvent() can only retrieve events for the current window. This problem doesn't have much to to with the rest, so I created a separate patch. The patch makes nsIEventStateManager use a static variable to store the current event. I've checked for other callers - nsGlobalWindow is the only one, no problem then.
Attachment #156891 - Flags: review?(danm.moz)
Attached patch Patch v3 (obsolete) (deleted) — — Splinter Review
Ok, added the changes to fix this form submission thing so we have something concrete to talk about. The changes seem somewhat ugly to me but I don't see a real alternative. Other callers of nsILinkHandler::OnLinkClick(Sync): * IEDocument::Navigate(): I think this is only called on link clicks and we don't have an event here anyway, so it should be ok to skip popup blocking. * nsGenericElement::TriggerLink(): Called on link clicks, user-initiated event then. * nsImageFrame::TriggerLink(): Same as above but with an image map instead of a plain link * nsPluginInstanceOwner::GetURL(): This is bug 176079 and bug 251793 * nsIsIndexFrame::OnSubmit(): an isindex tag can't be submitted other than by the user, right? Tested JS-generated key events - they don't work. So none of the other callers needs to check whether is has been triggered by a user initiated event (correct?).
Attachment #156889 - Attachment is obsolete: true
Attachment #156978 - Flags: review?(danm.moz)
Met with danm, jst, and bryner, and we talked about a general scheme like the patches being developed here (thanks, Wladimir) that should handle all cases: 1. Keep a counter, sEnablePopupLevel, in DOM code, incremented and decremented by some static methods on a class, or extern functions, or whatever. 2. Add a flag, mEventEnablesPopups, on nsDOMEvent, that we set appropriately: false always for most events, true for user-generated clicks, true for events generated by user or script if sEnablePopupLevel is non-zero. 3. When dispatching an event, if mEventEnablesPopups, increment sEnablePopupLevel before dispatching, and decrement immediately after the handler dispatch returns. 4. Add a similar flag to the dom/src/base/nsGlobalWindow.h nsTimeoutImpl struct that is set true if sEnablePopupLevel is non-zero, false otherwise. 5. In nsGlobalWindowImpl::RunTimeout, if the nsTimeoutImpl's flag is true, increment sEnablePopupLevel before dispatching the timeout/interval function or evaluating the timeout/interval expression, and decrement immediately after the handler returns. 6&7. Do the same sort of counter=> flag and flag => counter translation in the docshell's link-click plevent constructor, and dispatch code, respectively. Ok, what is wrong with this picture. /be (Note this is similar to the work Wladimir has been doing but extended to timeouts and asynchronous events and made more general, I believe. I haven't completely digested Wladimir's patch re: form submissions et.al.) /danm (Dan, don't forget to file that unrelated DDE bug! /be)
1. From what I see the value of this counter will be always 1 or 0 - that's why I made it a boolean flag. But sure, a counter is more stable if other features are added later. Should this still reside in nsPIDOMWindow (static function in an interface might be not that nice) or is there a better place for it? 2&3. This is not too difficult to do but lots of files will have to be changed. Is there a chance for it to get reviewed fast? The other problem is that this bug has a blocking-aviary1.0 flag. I'm creating a patch for the trunk, it will have to be ported to the Firefox branch and this needs to happen soon. I can't port anything to FF, so maybe it is wiser to do the small changes first and to keep the global ones for later (and for other bugs) - unless there is someone to do the porting before RC2 comes out (better even before RC1). One more point: 8. Remove the code blocking popups before the page has been loaded (bug 101190, actually) as it is a major annoyance and isn't necessary with all the changes made. The only popups it can still prevent are popups from plugins such as Flash - but these can just as easily open popups after the page has been loaded. Alltogether the changes are not too difficult to implement. If I were to do this all at once, I would need only a couple of days (most of it for testing). But reviewing it will be somewhat problematic. So, is there a chance to get this through? Btw: I've noticed that attachment 156891 [details] [diff] [review] is actually a patch for bug 240246 (the testcase there is overcomplicated but it is exactly the same problem) - should I move it there?
I ran into trouble when rewriting my patch according to Brendan's suggestions. It's about sEnablePopupLevel being accessible through static functions or static methods. This doesn't work when used from different libraries as we have it here (layout and docshell at the very least). One solution would be to have non-static context modify the static variable - the window object, as I had it before. The other way would be to move it into an XPCOM service, maybe nsIPopupWindowManager. I tend to use the first solution.
(In reply to comment #27) 4&5: This makes abuse of click events even easier (see bug 197919 comment 18 and following). Consider this: <body onclick="setInterval('window.open()', 1000)"> Don't dare click on me!!! </body> I think we need a way to restrict the number of popups opened by one event (and all the events and timeouts it produced). We need a popup counter object then. I'm thinking of interface changes like these: public class nsPIDOMWindow : public nsIDOMWindowInternal { ... // Returns the current popup counter or creates a new one if // there is none. virtual nsIPopupCounter* GetPopupCounter(); // Sets the current popup counter, that will be used to determine // whether popups are allowed. If the parameter is nsnull, popups // will be always disallowed. virtual void SetPopupCounter(nsIPopupCounter* counter); // Tests whether popup windows are allowed with the current popup // counter. virtual PRBool ArePopupsAllowed(); } class GlobalWindowImpl : ..., public nsPIDOMWindow { ... protected: static nsIPopupCounter sCurrentPopupCounter; } public class nsIPopupCounter : public nsISupports { public: // Sets this object the current popup counter, thus allowing popups // unless the maximum popup number for this counter has been already // exceeded. void EnablePopups(); // Restores the current popup counter, usually setting it nsnull and // thus disallowing any popups. void DisablePopups(); // Increments the popup counter. void IncrementPopupCount(); // Tests whether popup windows are still allowed or the number of // open popups already reached the limit set in dom.popup_maximum_per_event. PRBool ArePopupsAllowed(); protected: PRInt32 mEnablePopupLevel; PRInt32 mPopupCount; nsIPopupCounter* mRestorePopupCounter; } Instead of a field mEventEnablesPopups we will have a field mPopupCounter in the events then. For user-initiated events or when window->ArePopupsEnable() returns PR_TRUE we will fill it with the result of window->GetPopupCounter(), otherwise it will be nsnull. Same thing for timeouts. I think, dom.popup_maximum_per_event shouldn't be more then 5, three popups per event might be an appropriate value.
I'm not keen on that. The last time this issue was raised we decided instead to limit the total number of popups. That pref exists and works but is set kind of high by default because there's no UI for adjusting it. Now if you wanted to write a popup blocker UI, that's sorely missed and no one's working on adding one. Though I suppose it's too late for that in 1.0.
Attached patch Work in progress (untested) (obsolete) (deleted) — — Splinter Review
This implements the suggestions so far except for 2&3 (event handling not yet touched). I'll attach the two new files in a moment. Completely untested, I don't know whether this works or not.
Attached file Work in progress (dom/src/base/nsPopupCounter.cpp) (obsolete) (deleted) —
Attached file Work in progress (dom/public/base/nsIPopupCounter.h) (obsolete) (deleted) —
Attachment #156978 - Flags: review?(danm.moz)
Why a new nsIPopupCounter interface? Can't we simply add those methods to nsPIDOMWindow?
(In reply to comment #35) The current (working) version has no interface nsIPopupCounter anymore, just an nsPopupCounter class. But this one is necessary if we want to have a popup limit per user action as proposed in comment 30. We have to share one nsPopupCounter instance between all events and timeouts created by the same user action. Other events and timeouts can have another instance of nsPopupCounter at the same time. (In reply to comment #31) The way I interpret the code dom.popup_maximum doesn't limit popups created by click events anyway? That's what the attachment in bug 197919 comment 80 should demonstrate, I think...
> it will have to be ported to the Firefox branch And the 1.7 branch, right?
Generally I have it all working but there is a big problem - there are at least a dozen places in the Mozilla code turning untrusted events into trusted ones. Example is the HTML button element that makes a trusted click event out of an untrusted keypress event with keyCode == 13. This seems not to be security relevant (spent the last two days figuring this out) but it makes the popup blocker useless if implemented the way Brendan's suggestions 2 and 3 go. The current popup blocker is not affected because these events don't go through nsPresShell::HandleEvent() (HandleDOMEvent() instead). So we could mark every event (nsEvent, not DOM event) that went through nsPresShell::HandleEvent() with a special flag that can be used to determine which events are really trustable. This leaves us with the problem of secondary events. If the user really hits the return key on a button, we want the resulting click event to enable popups as well. We don't get a real chance to set any global flags though, when working on the nsDOMEvent level... Other solution would be to move the whole processing into nsEvent instead of nsDOMEvent. This will block some chances to improve the whole thing, but it should work. And we could fix the original problem of course, so that no untrusted event can create a trusted one. But this seems to be quite a job to do... Btw: The build system is making troubles as well. Brendan, are you sure you meant "DOM code" in your first suggestion? Sure these exported static functions shouldn't live in the XPCOM code?
Attached file Work in progress (obsolete) (deleted) —
Works except for the problem mentioned above and the building (docshell has to depend on gklayout, don't know the proper way to do this).
Attachment #157172 - Attachment is obsolete: true
Attachment #157173 - Attachment is obsolete: true
Attachment #157174 - Attachment is obsolete: true
The way I see it, the first two solutions from comment 38 would require huge and unreliable hacks. The third is the only real solution but it requires additional checks for trustable events at 18 places in the code. Any comments, someone? Btw, I was wrong when saying that the current popup blocker implementation wasn't affected. I found three new ways to trick it using problems in the event handling. I'm sure somebody would have found them already if there were no easier ways to do it...
Why is having a "trusted" boolean in every nsEvent "unreliable"? In my opinion that's absolutely the way to go. People who create nsEvent structs should specify whether the event is a trusted one; since they're creating it, they should know. This is required for solution #3 anyway, no?
try for PR
Flags: blocking-aviary1.0PR- → blocking-aviary1.0PR+
(In reply to comment #41) > Why is having a "trusted" boolean in every nsEvent "unreliable"? In my opinion > that's absolutely the way to go. People who create nsEvent structs should > specify whether the event is a trusted one; since they're creating it, they > should know. Yes, I came to the same conclusion (though I made it a "not trusted" flag instead of a boolean). "Unreliable" was refering to setting this flag in nsPresShell::HandleEvent(), which was an utterly bad idea. So I have to set it when the events are created, that's what I'm on right now (requires lots of testing).
Ok, there is obviously a case when people creating an event don't know whether it can be trusted. The change event is created by a focus change - and this is sometimes several calls away from the originating event (that might be not trustable). If I were to forward the originating event through the chain of calls, I would have to change nearly every file in layout and content and some in other directories. Suggestion: non-trusted events shouldn't be able to change focus at all, so that any SetFocus() call can be considered trusted. Will it break something? (In reply to comment #42) > try for PR This doesn't look too likely...
Flags: blocking-aviary1.0PR+ → blocking-aviary1.0PR-
ccing some people who know about focus.
Wladimir, I'm working on a version of this for the trunk that doesn't impact as much code. Find me on #mozilla (jst) if you've got some cycles to talk about this.
I've split the changes into five independent parts, four of them I have finished. The missing part should prevent abuse of the change event, select and input should be desirable as well (those are not popup-enabling by default though). At the current stage it is still possible to open popups through some tricks using the change event, but it is already better than the current implementation. If it gets reviewed and ported in time, it might be worth considering checking it in without waiting for the missing part. I've looked at jst's solution and it is very similar. However I had more time testing and I am almost sure that I have the propagation of the trusted flag correct for all relevant events but NS_FORM_CHANGED - meaning that I won't block any user-requested popup and allow only a few of the not requested.
Attachment #156891 - Attachment is obsolete: true
Attachment #156978 - Attachment is obsolete: true
Attachment #157426 - Attachment is obsolete: true
Attachment #156891 - Flags: review?(danm.moz)
This accomplishes it for the events NS_MOUSE_LEFT_*, NS_FORM_SUBMIT, NS_FORM_RESET and NS_UI_ACTIVATE. For NS_KEY_* there was nothing to do. Part 3 should take care of NS_FORM_CHANGE and, if possible, of NS_FORM_SELECTED and NS_FORM_INPUT. This still has to be done.
Attached patch Part 4: The actual popup blocker patch (obsolete) (deleted) — — Splinter Review
This class isn't used anymore and I don't think it will be useful somewhere else given that it only catches the events that are going through nsPresShell::HandleEvent().
Wrong file attached, sorry about the spam...
Attachment #157740 - Attachment is obsolete: true
Attachment #157738 - Flags: review?(jst)
Attachment #157739 - Flags: review?(jst)
Attachment #157741 - Flags: review?(jst)
Attachment #157742 - Flags: review?(jst)
I haven't read the patch in detail, but it does seem like the right approach to me, overall. Some comments offhand: 1) I'd make the "trusted" flag an arg to the nsEvent constructor. As things stand, people can easily create trusted events by accident. 2) I think the right number of popups allowed per event is 1, not 5. 3) Munging nsEvent flags by hand seems painful; maybe we want a getter and setter for the "not trusted" flag, for readability.
> As things stand, people can easily create trusted events by accident. Case in point: the changes to nsXULElement in attachment 157739 [details] [diff] [review] are wrong, such that the mouseup and click events are ending up trusted. It also looks to me like a click (or submit, or change) event handler that calls form.submit() will be blocked from popping up a window (if the form's target is "_blank") by the new cod, because the submit() call drops privileges. I'm not sure what the best way of addressing this is, but it needs to be addressed -- that code pattern is very common.
Attached patch Alternative approach (aviary branch) (obsolete) (deleted) — — Splinter Review
This is an alternative approach to fixing this bug, at least partly. This is similar to Wladimirs changes, yet different in some ways. We made the same type of change to nsEvent, to have a flag that says whether the event is trusted or not, this patch defaults all events to non-trusted events, and only sets those events that come through nsViewManager::DispatchEvent() as trusted events, as the only code that calls that is the native widget event that dispatches OS events into Mozilla code (AFAICT). This approach is a more brute-force way of blocking events, it basically says that no popups are allowed, except in some cases. And those cases are set by the event listener manager and the pres shell, as that's where all (well, most) events go through. This keeps the event name checking code from the old code, but moves it to nsDOMEvent (and changes it some), and also changes the criteria for when certain events enable popups. For instance, submit, change, input etc events only enable popups if they're dispatched while handling user input events (mouse clicks, key presses, see nsPresShell for details). So far I haven't been able to break this approach, except by hooking into a click handler or some other handler where we explicitly enable popups. But there's never ever going to be a way to reall fix those cases. There's still some cleaning up that needs to happen here, but in general, this is close to where I wanted to go with this.
if we are going to do this for 1.0 we need to get into PR.
Flags: blocking-aviary1.0PR-
Flags: blocking-aviary1.0PR+
Flags: blocking-aviary1.0-
Flags: blocking-aviary1.0+
Hmm... Looking at jst's patch here. I assume if I change the value of a dropdown with mouse the change event will fire while the "handling user input" flag from the mouseclick or mouseup is set? And that a change event that fires due to the page calling blur() after I've done some editing will not be able to open popups? Overall, the approach seems ok, and possibly safe enough to land on branch, though it would help to have a reasonably comprehensive test suite of things that should and should not work. One thing that worries me is that early returns can leave us in the "handling user input" state forever; it's probably better to use an nsAuto* thing of some sort to handle that too. One thing is that this _will_ prevent popups on sites that request expanded privileges (whereas Wladimir's patch does not). For branch that's ok, and I'm not really sure which direction I'd prefer going forward...
(In reply to comment #52) > 1) I'd make the "trusted" flag an arg to the nsEvent constructor. As things > stand, people can easily create trusted events by accident. Yes, but for now it needs to have a default value, namely PR_TRUE. We can't change every event creation in the codebase. And making PR_FALSE the default value will break web-apps (as happens with jst's approach - change events are almost never allowed to open popups). We can do it later, when we have the problems with the change events sorted out (which requires quite a huge patch again). > 2) I think the right number of popups allowed per event is 1, not 5. Agree but we don't have an UI to change this so we have to set a high default value. My suggestion was 3 - I can't imagine a web-app that needs to open more than three popups at the same time (though 2 are definitely possible, so maybe three as well). > 3) Munging nsEvent flags by hand seems painful; maybe we want a getter and setter > for the "not trusted" flag, for readability. Yes, the flag has to be moved into internalAppFlags anyway, I will make it use getter and setter then. (In reply to comment #53) > It also looks to me like a click (or submit, or change) event handler that > calls form.submit() will be blocked from popping up a window (if the form's > target is "_blank") by the new cod, because the submit() call drops > privileges. That's not the case. While the NS_FORM_SUBMIT event created is (correctly) not trusted, we are enabling popups for the onclick handler. The form submission will take the active popup counter, the one created for the click event in this case.
Attached patch Alternative approach updated (aviary branch) (obsolete) (deleted) — — Splinter Review
This fixes a bunch of stuff, focus stuff should be all done, form events etc all done. I'll attach a trunk version of this as well.
Attachment #157751 - Attachment is obsolete: true
Attached patch Alternative approach (trunk) (obsolete) (deleted) — — Splinter Review
Same thing for the trunk.
Attachment #157738 - Flags: review?(jst)
Attachment #157739 - Flags: review?(jst)
Attachment #157741 - Flags: review?(jst)
Attachment #157742 - Flags: review?(jst)
Very nice! This is definitely the better solution. I'm testing it and so far there is only one problem, in nsGlobalWindow.cpp: static PRBool IsPopupBlocked(nsIDOMDocument* aDoc) { - PRBool blocked = PR_FALSE; + PRBool blocked = PR_TRUE; nsCOMPtr<nsIDocument> doc(do_QueryInterface(aDoc)); nsCOMPtr<nsIPopupWindowManager> pm(do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID)); if (pm && doc) { Otherwise this code would open a popup because the frame doesn't have a document yet: <body> <iframe></iframe> <script>frames[0].open();</script> </body> Your change of nsGenericHTMLElement.cpp seems to be a restover from the previous patch version, IMO it isn't necessary. Scripts cannot create NS_GOTFOCUS, NS_LOSTFOCUS, NS_ACTIVATE and NS_DEACTIVATE events, neither directly nor indirectly. I think you can drop the caller check for these events in PresShell::HandleEventInternal(). And are there any other events than the ones listed going through pres shell? It seems to me that you can generally push PR_TRUE as UserInputState for trusted events.
> We can't change every event creation in the codebase. Actually, changing every event creation is exactly what I was thinking (though this may be superceded by jst's approach). > Scripts cannot create NS_GOTFOCUS, NS_LOSTFOCUS, NS_ACTIVATE and NS_DEACTIVATE So a focus() call from a script won't cause those events? What if I focus something in one window and then the script calls focus() on something in another window? (I just don't know much about these events; pardon my ignorance.)
(In reply to comment #61) > > Scripts cannot create NS_GOTFOCUS, NS_LOSTFOCUS, NS_ACTIVATE and NS_DEACTIVATE > > So a focus() call from a script won't cause those events? What if I focus > something in one window and then the script calls focus() on something in > another window? (I just don't know much about these events; pardon my ignorance.) Wladimir told me to ignore his last paragraph, which was this one. He tested and realized that what he stated wasn't the case.
Attached patch Also deal with timeouts (obsolete) (deleted) — — Splinter Review
This makes us propagate popup state to timouts in setTimout/interval *if* not called from within another timeout and if the timeout delay is less than what's set in the "dom.disable_open_click_delay" pref (which admittedly is now misnamed, but I decided to keep the name for some level of backwards compat), and then when a timeout fires, we clear the popup state to prevent repeated abuse. This means that any timeout created when popups are enabled will be able to open popups the first time they fire if their delay is less than what's set in the pref. Seems like that's desirable behavour.
Attachment #157785 - Attachment is obsolete: true
Attachment #157786 - Attachment is obsolete: true
Attached patch Also deal with timeouts (trunk version) (obsolete) (deleted) — — Splinter Review
Blocks: 101190
Attachment #157822 - Flags: review?(bzbarsky)
(In reply to comment #56) > One thing is that this _will_ prevent popups on sites that request expanded > privileges (whereas Wladimir's patch does not). For branch that's ok, and I'm > not really sure which direction I'd prefer going forward... For the record, bz and I talked about this last night and the two approaches are no different in this respect, but neither nails this correctly all the time.
Oh, and as Wladimir pointed out, the nsGenericHTMLElement.cpp change is not needed, so ignore those in the diff.
Attachment #157822 - Flags: superreview?(brendan)
Comment on attachment 157822 [details] [diff] [review] Also deal with timeouts (trunk version) > case NS_KEY_EVENT : { No need to brace the case's code here. > // its delay (interval) is less than what "not greater than" or "less than or equal to" Given gRunningTimeoutDepth, do we need mTimeoutFiringDepth still? > nsPIDOMWindow *window = timeout->mPopupState < openAbused ? this : nsnull; Style nit: parenthesize ? left operand unless it's unary or primary? Old Unix god style. Quick comments only, I'll wait for bz's r= to sr=. /be
Comment on attachment 157822 [details] [diff] [review] Also deal with timeouts (trunk version) >Index: content/events/src/nsDOMEvent.cpp >+PopupAllowedForEvent(const char *eventName) >+ nsAFlatCString::const_iterator start, end; >+ events.BeginReading(start); >+ nsAFlatCString::const_iterator startiter(start); I know you just copied this, but those last two lines can be replaced with: nsAFlatCString::const_iterator startiter(events.BeginReading(start)); >+nsDOMEvent::GetEventPopupControlState(nsEvent *aEvent) >+ // triggerd while handling user input. See "triggered" >Index: content/events/src/nsEventListenerManager.cpp >@@ -1576,18 +1578,30 @@ nsresult nsEventListenerManager::HandleE >+ if (aPresContext) { What about events dispatched in display:none iframes? Maybe we want to get the document off the target in those cases? Or just in general? >Index: dom/public/base/nsPIDOMWindow.h >+enum PopupControlState { Please document that the values have to go from "most permissive" to "least permissive" so the greater-than check in PushPopupControlState() will do the right thing. >+class nsAutoPopupStatePusher >+ nsPIDOMWindow *mWindow; // WEAK Is this OK? This is going to be held across event processing; what if window.close() is called? Or do we not "really close" the window for a bit until we've returned to the main event loop? I have to admit that I'm not sure why pushing and popping the popup state (which only changes a static member) is done through instance member functions. Wouldn't using static members (like you do for the ESM) work better? Then there would be no need to pass actual window objects to nsAutoPopupStatePusher either... Is the problem that we need these to be functions on nsPIDOMWindow so we can call them from outside the content module and we can't do that with statics? If so, could we push the static member functions (and the static boolean member) up to nsPIDOMindow? >Index: dom/src/base/nsGlobalWindow.cpp > PRBool IsPopupBlocked(nsIDOMDocument* aDoc) > { >- PRBool blocked = PR_FALSE; >+ PRBool blocked = PR_TRUE; This will block all popups if there is no popup manager? But the popup manager is an extension, and its interface is not frozen. So we can't reasonably expect embeddors to implement it.... Was there a reason for this change? >@@ -4888,18 +4720,35 @@ GlobalWindowImpl::SetTimeoutOrInterval(P >+ if (interval <= (delay * 1000)) { How about PR_MSEC_PER_SEC instead of "1000" so it's clear what units are involved in this conversion? >@@ -5007,22 +4856,33 @@ GlobalWindowImpl::RunTimeout(nsTimeoutIm >+ // If the popups are enabled for this timeout, push its popup s/the popups/popups/ >+ nsPIDOMWindow *window = timeout->mPopupState < openAbused ? this : nsnull; Why do you need this test? Won't the less-than test in PushPopupControlState do the same thing already? >+ // Clear the timeouts popup state, if any, to prevent interval "timeout's" >Index: layout/base/public/nsPresContext.h >- nsIDocument* GetDocument() { return GetPresShell()->GetDocument(); } >+ nsIDocument* GetDocument() { return mShell ? mShell->GetDocument() : nsnull; Why this change? I thought the point was to move to where a pres context would always have a presshell... are you hitting a case when that's not happening? Leaving with an "r?" request for now, pending answers to some of the questions above.
Comment on attachment 157822 [details] [diff] [review] Also deal with timeouts (trunk version) r=bzbarsky with the changes we discussed on irc.
Attachment #157822 - Flags: review?(bzbarsky) → review+
Found another problem with this patch, it is breaking the access keys (the current popup blocker does as well, though not as consistently). Three test cases, that fail to open a requested popup: <a href="#" onclick="window.open();return false;" accesskey="a">Press Alt-A to open a popup</a> <a href="javascript:void window.open()" accesskey="b">Press Alt-B to open a popup</a> <form action="javascript:void window.open()"> <label for="submit" accesskey="c">Press Alt-C to open a popup</label> <input id="submit" type="submit"> </form> The first one could be fixed with a change in nsDOMEvent.cpp: case NS_KEY_EVENT : { - if (aEvent->internalAppFlags & NS_APP_EVENT_FLAG_TRUSTED) { + if (aEvent->internalAppFlags & NS_APP_EVENT_FLAG_TRUSTED || + nsEventStateManager::IsHandlingUserInput()) { ... case NS_MOUSE_EVENT : - if (aEvent->internalAppFlags & NS_APP_EVENT_FLAG_TRUSTED) { + if (aEvent->internalAppFlags & NS_APP_EVENT_FLAG_TRUSTED || + nsEventStateManager::IsHandlingUserInput()) { This will permit non-trusted mouse and keyboard events if they have been created handling a user action without any JavaScript event handlers. The other two testcases are more fun. My suggestion would be to change nsWebShell.cpp. You could save UserInputState instead of PopupControlState in OnLinkClick(). OnLinkClickSync() would check IsHandlingUserInput() then and push openAllowed onto stack if true is returned. After all, it is a click and it cannot get openControlled regardless the event that produced it. Ok, and now the minor things. In nsDOMEvent.cpp you can throw out includes for nsContentUtils.h and nsPIDOMWindow.h - the first one is loaded two lines above, the second in nsDOMEvent.h. Same file, GetEventPopupControlState() - NS_RESIZE_EVENT can be dropped, it is not detectable whether this is user-initiated (the mouse events resizing a window go to the window border, we don't get them). nsHTMLFormElement.cpp: >+ nsCOMPtr<nsPIDOMWindow> window = >+ do_QueryInterface(nsGenericElement::GetOwnerDoc()-> >+ GetScriptGlobalObject()); Is there any point in calling nsGenericElement explicitely here? First parameter of GlobalWindowImpl::CheckOpenAllow() should have type PopupControlState. XPCDocument.cpp: > #include <mshtml.h> > #include <hlink.h> >+#include "nsPIDOMWindow.h" The includes for XPCOM interfaces are located ~30 lines further down, you should probably move this one...
Attached patch Address reviewer comments (trunk version). (obsolete) (deleted) — — Splinter Review
Attachment #157821 - Attachment is obsolete: true
Attachment #157822 - Attachment is obsolete: true
Attachment #157738 - Attachment is obsolete: true
Attachment #157739 - Attachment is obsolete: true
Attachment #157899 - Attachment is obsolete: true
Fix checked in on the trunk. And I also set "dom.disable_open_click_delay" to 1 to enable popups from timeouts that fire in < 1s if initiated when popups are enabled. Forgot to include that in all the diffs here.
Comment on attachment 157902 [details] [diff] [review] Address reviewer comments (trunk version). sr=me, already communicated face to face. A couple of missing apostrophe nits still in this patch: // permissive to least permissive so that its safe to push state in // Push this timeouts popup control state, which should only be Fix those for good measure. /be
Attachment #157902 - Flags: superreview+
Attachment #157822 - Flags: superreview?(brendan)
Whiteboard: [sg:nse,fix] → [sg:nse,fix] [have patch] ready for checkin
Am I wrong that http://bugzilla.mozilla.org/attachment.cgi?id=157898&action=view is the proper patch for the aviary branch (comment #71)? Then this is still missing s/sr/a? Or did Brendan's last comment include a sr for 157898 as well? I ask because this is still marked blocking 1.0 PR.
(In reply to comment #74) > Fix checked in on the trunk. And I also set "dom.disable_open_click_delay" to 1 > to enable popups from timeouts that fire in < 1s if initiated when popups are > enabled. You know that these pref is in milliseconds? See bug 126224 comment 7. Are you sure to use 1 as default value in all.js? I guess you mean 1000. http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/init/all.js#416
Johnny's patch changes that pref to be in seconds, not milliseconds (did you even read the diff?). Whether that's desirable is a separate issue.
Yeah, the pref is now in seconds, not milliseconds. That's easy to change though, but I can't imagine anyone needing finer grained control than seconds for this pref, really. But I'm happy to change this if people think there's more value in having this pref be in ms.
(In reply to comment #78) > Johnny's patch changes that pref to be in seconds, not milliseconds (did you > even read the diff?). I did read the diff, but I overlooked the multiplication with PR_MSEC_PER_SEC (Note: There was no comment that indicates that change in this report or patch). (In reply to comment #79) > Yeah, the pref is now in seconds, not milliseconds. That's easy to change > though, but I can't imagine anyone needing finer grained control than seconds > for this pref, really. Just my thoughts ... 1) Backwards compatibility (see your comment 63): A couple of people still use this pref and have set it as an user_pref in user.js/pref.js in milliseconds. 2) The pref is hidden and internally you convert it to milliseconds anyway. So I see no real benefit to change the unit.
(In reply to comment #80) > Just my thoughts ... 1) Backwards compatibility (see your comment 63): A couple > of people still use this pref and have set it as an user_pref in user.js/pref.js > in milliseconds. 2) The pref is hidden and internally you convert it to > milliseconds anyway. So I see no real benefit to change the unit. Ah, now I see what happened here. I was *trying* to make this backwards compatible, but I screwed up and didn't realize that mLastMouseButtonAction was in us, not ms, thus I assumed that the pref was always in s, not ms. Duh. I fixed the problem, made the pref default to 1000, i.e. 1000ms. Thanks for spotting this problem!
Wladimirs attachment 157741 [details] [diff] [review] is now checked in on the trunk (Part 5: Remove CurrentEventShepherd from EventStateManager).
Fixed on the aviary branch now too. Are we ready to close this bug?
Keywords: fixed-aviary1.0
this affecting gecko, shouldn't it land on 1.7 too?
We can close the bug (since it's fixed on trunk), as long as this is flagged for checkin on the 1.7 branch and so forth (do we have such flags? If not, we clearly need them).
Comment on attachment 158155 [details] [diff] [review] Final aviary diff for checkin (includes post checkin fixes from the trunk) We could go either way... but I don't see any reason *not* to improve the popup blocker on the 1.7 branch as well, assuming it doesn't cause any problems...
Attachment #158155 - Flags: superreview+
Attachment #158155 - Flags: review+
Attachment #158155 - Flags: approval1.7.x?
Comment on attachment 158155 [details] [diff] [review] Final aviary diff for checkin (includes post checkin fixes from the trunk) We could go either way... but I don't see any reason *not* to improve the popup blocker on the 1.7 branch as well, assuming it doesn't cause any problems...
Comment on attachment 158155 [details] [diff] [review] Final aviary diff for checkin (includes post checkin fixes from the trunk) a=dveditz for 1.7
Attachment #158155 - Flags: approval1.7.x?
Attachment #158155 - Flags: approval1.7.x+
Attached patch Block popups opened by plugins (obsolete) (deleted) — — Splinter Review
It seems that plugins are the weakest point of the popup blocker now - they are always allowed to open popups (bug 176079). This patch makes this behavior configurable - if we have privacy.popups.disable_from_plugins set to true, any popups opened by a plugin will be blocked unless whitelisted. The pref can be enabled by default later if necessary (UI would be also nice). Tested on http://www.pirates.bethsoft.com/flash/fbplus.html - works.
Comment on attachment 158176 [details] [diff] [review] Block popups opened by plugins Forgot to mention: this is a trunk patch but it should be applicable on aviary as well.
Attachment #158176 - Flags: review?(jst)
Attached patch Block popups opened by plugins (obsolete) (deleted) — — Splinter Review
It should be openControlled instead of openAllowed for popups opened by plugins so that the limit for the number of open popups still applies even if the plugins are allowed to open popups (killed my browser on the example from bug 176079 comment 5, that helped realizing it).
Attachment #158176 - Attachment is obsolete: true
Attachment #158178 - Flags: review?(jst)
Attachment #158176 - Flags: review?(jst)
Blocks: 258475
Filed bug 258487 on the plugin issue. Let's close this one and followup with new bugs as needed.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
The checkin for this caused bug 258499
Blocks: 258499
(In reply to comment #86) > We can close the bug (since it's fixed on trunk), as long as this is flagged for > checkin on the 1.7 branch and so forth (do we have such flags? If not, we > clearly need them). bz, that would be any bug that has a patch marked approval1.7+ and does *not* have a fixed1.7 keyword in it, like this bug...
Ah, ok. What exactly is blocking landing this on the 1.7 branch anyway?
Just the fact that I haven't had the time to merge and check in, nothing else...
Attachment #158178 - Flags: review?(jst)
There really should be no merging required... if the two branches are being kept in sync (as they should be), the patch should just apply. Do you want me to land this on 1.7 when I get back? I could do that.
bz: I heard dbaron had done some trial joins of back end code from aviary branch to 1.7 branch, and wanted to do a consolidated patch. Doing pieces may help him, or could conceivably cause conflicts (or perhaps just "already contains revisions" noise?). Maybe we should convene on IRC later today and figure out a plan? /be
Any chance this caused Bug 259117?
Indeed it did.
Blocks: 259117
Blocks: 238457
Is this going into the 1.7 branch?
Flags: blocking1.7.x?
Yes, if this landed on Aviary (it did) then it needs to land on the 1.7 branch. a=dveditz for the 1.7 branch
Flags: blocking1.7.x? → blocking1.7.x+
Whiteboard: [sg:nse,fix] [have patch] ready for checkin → [sg:nse,fix] needed-1.7.x
Fixed on the 1.7 branch now too.
Keywords: fixed1.7.x
Whiteboard: [sg:nse,fix] needed-1.7.x → [sg:nse,fix]
The Tru64 UNIX compiler/linker doesn't like the code: static void operator delete(void* /*memory*/); which is in the class nsAutoHandlingUserInputStatePusher in the file content/events/src/nsEventStateManager.h I would suggest changing it to: static void operator delete(void* /*memory*/) {} which will make the Tru64 UNIX linker link the symbols into the libgklayout.so. We can "#ifdef OSF1" it if it is a problem for other OSs. Without this change I get the following error while loading the library libgklayout.so: "480496:./mozilla-bin: /sbin/loader: Error: Unresolved symbol in /usr4/dailybld/ mozilla/dist/bin/components/libgklayout.so: __dl__34nsAutoHandlingUserInputState PusherXPv nsNativeComponentLoader: SelfRegisterDll(libgklayout.so) Load FAILED with error: dlopen: Unresolved symbols".
I'd put in an assert in the implementation as well to make sure it's not called if it's not meant to be. I've seen this same thing in VC++ 6 when a class is used in a certain way. Best to provide an implementation that fails.
Fixed. I didn't add an assertion as we don't do that in any other places in Mozilla code where we're hiding operator new/delete (but that wouldn't be a bad thing to do...).
Would be interesting to look through the generated asm and see where the reference comes from.
Based on circumstantial evidence, I have good reason to believe that the fix for this bug on the Aviary branch (attachment 158155 [details] [diff] [review]) is responsible for (topcrash) bug 255372 (see bug 255372 comment 58). It would be nice if someone who understands the patch can take a look. Apologies if I'm blaming the innocent.
I was wrong in my previous comment. I now see that the regression occured before the patch to this bug was checked in. My apologies from the previous comment apply (especially to Wladimir). Additional apologies to everyone for the spam.
Note: this blocks opening links in a new window when triggered via accesskey (bug 268830).
Product: Core → Mozilla Application Suite
this broke javascript: bookmarklets opening a new window in camino (bug 272389)
Blocks: 272389
No longer blocks: 272389
Attached file mochitest compatible testcase (deleted) —
I converted some of my tests for the MochiTest framework - the ones that don't require user interaction. Altogether this is lots of code that also takes a few seconds to run but I think this is necessary to test all the combinations (indirect event creation, execution during page load, delay by timeouts etc). This tries to open 99 popups for me but the number can differ - e.g. some of the indirect event creation methods used to work in Gecko 1.7 or 1.8 but don't work on trunk now.
Attachment #158178 - Attachment is obsolete: true
Attachment #247067 - Flags: review?(jst)
Attachment #247067 - Attachment mime type: text/html → application/xhtml+xml
Attachment #247067 - Flags: review?(jst) → review+
Whiteboard: [sg:nse,fix] → [sg:nse,fix] longtest
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: