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: jwkbugzilla)

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
28 bytes, text/javascript
Details
testcase
411 bytes, text/html
Details
shows both onload and setTimeout
486 bytes, text/html
Details
Part 5: Remove CurrentEventShepherd from EventStateManager
14.86 KB, patch
Details | Diff | Splinter Review
157740: Part 4: The actual popup blocker patch
70.38 KB, patch
Details | Diff | Splinter Review
Address reviewer comments.
72.42 KB, patch
Details | Diff | Splinter Review
Address reviewer comments (trunk version).
77.66 KB, patch
brendan
: superreview+
Details | Diff | Splinter Review
Final aviary diff for checkin (includes post checkin fixes from the trunk)
73.40 KB, patch
jst
: review+
jst
: review+
jst
: superreview+
jst
: superreview+
dveditz
: approval1.7.5+
Details | Diff | Splinter Review
mochitest compatible testcase
15.11 KB, 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
          
        testcase
        — 
      

    


  

    
  
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 :-)

    
    

    
  
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)
        — 
          — 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)
        — 
          — 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

    
    

    
  


  
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)
        — 
          — 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)
        — 
          — 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.

    
    

    
  


  

    
    

    
  


  

    
  

        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)
        — 
      

    


  
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.

    
    

    
  


  

    
    

    
  


  
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.

    
    

    
  


  
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.

    
    

    
  


  
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)
        — 
          — 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)
        — 
          — 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
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...

    
    

    
  


  

        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)
        — 
          — 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)
        — 
          — 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

    
    

    
  


  
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: