Last Comment Bug 463491 - When save dialog is open in pop up blocker does not work at all
: When save dialog is open in pop up blocker does not work at all
Status: REOPENED
: testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 662519 678166 679961
Blocks: popups
  Show dependency treegraph
 
Reported: 2008-11-06 12:51 PST by Amoor Avakian
Modified: 2013-10-03 13:27 PDT (History)
15 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
wontfix


Attachments
testcase (777 bytes, text/html)
2011-06-06 06:42 PDT, Mounir Lamouri (:mounir)
no flags Details
Patch v1 (5.04 KB, patch)
2011-06-07 09:27 PDT, Mounir Lamouri (:mounir)
jst: review+
Details | Diff | Splinter Review
Patch with updated tests (4.71 KB, patch)
2011-08-09 16:39 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Amoor Avakian 2008-11-06 12:51:26 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3 (.NET CLR 3.5.30729)

When (save) dialog is open in pop up blocker does not work at all and this is reproducible every time.

Reproducible: Always

Steps to Reproduce:
1. Turn on the pop-up blocker

2. Go to a site that has pop-ups that are done via JS over a period of time (that means they have to re-try, and hence not only on page load.

3. Pick a graphic or whatever to save from the site and leave the 'save dialog' open and wait for the JS to launch the popup.

4. The pop-up will be launch every time.
Actual Results:  
The pop-up's appear even though the blockers is enabled

Expected Results:  
no pop-ups
Comment 1 Tyler Downer [:Tyler] 2010-10-06 14:26:28 PDT
This is a mass search for bugs which are in the Firefox General component, are
UNCO, have not been changed for 500 days and have an unspecified version. 

Reporter, can you please update to Firefox 3.6.10 or later, create a fresh profile, http://support.mozilla.com/en-US/kb/managing+profiles, and test again. If you still see the issue, please update this bug. If the issue is gone, please set the status to RESOLVED > WORKSFORME.
Comment 2 Tyler Downer [:Tyler] 2010-11-01 22:59:48 PDT
No reply from reporter, INCOMPLETE. Please retest with Firefox 3.6.12 or later and a new profile (http://support.mozilla.com/kb/Managing+profiles). If you continue to see this issue with the newest firefox and a new profile, then please comment on this bug.
Comment 3 :aceman 2011-05-30 11:44:32 PDT
I can confirm this problem in Firefox 7.0a1 on the testcase in bug 659882. Javascript repeatedly tries to open a window checking if it got success. The popupblocker bar appears and counts up windows it killed. When using File->Save as (the whole page) and having the save dialog open, the JS window succeeds to open.
Comment 4 Mounir Lamouri (:mounir) 2011-06-06 06:42:16 PDT
Created attachment 537545 [details]
testcase
Comment 5 Mounir Lamouri (:mounir) 2011-06-06 09:02:22 PDT
The reason seems to be because the save and open dialogs are open through a XUL command event and those events are considered safe so the popup blocker level is reduced to "openControlled" thus making all popup to pass. So it doesn't happen when using an <input type='file'> which makes this bug less critical...

Though, I wonder why we need to check if the event is a XUL command and reduce the popup blocker in that case. Do we really want to have a popup blocker for chrome? I mean can't we just by-pass the popup-blocker check when we have chrome privileges?
Comment 6 Mounir Lamouri (:mounir) 2011-06-07 07:56:08 PDT
So, I'm going to fix that in two parts:
1. Make sure popup blocker is by-passed when the caller has chrome privileges (bug 662519);
2. Don't reduce the abuse level when the command event is used.

I could probably do 2 without 1 but the only reason we could be reducing the abuse level is that the popup blocker was sometimes interfering with the chrome/xul pages. 1 will make sure that this can't happen. In addition, there is no reason to use the popup blocker in the chrome context. At least, I don't see any...
Comment 7 Mounir Lamouri (:mounir) 2011-06-07 09:27:21 PDT
Created attachment 537808 [details] [diff] [review]
Patch v1
Comment 8 Mounir Lamouri (:mounir) 2011-06-07 09:30:53 PDT
Actually, my test is using "ctrlKey: true" but I wonder how would that behave on a Mac. Should I use metaKey in that case?
Comment 9 Mounir Lamouri (:mounir) 2011-07-18 18:25:32 PDT
Jst, any update regarding the review?
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-19 23:10:51 PDT
Comment on attachment 537808 [details] [diff] [review]
Patch v1

Sorry for the delay, I thought I had already reviewed this... :(

r=jst
Comment 11 Mounir Lamouri (:mounir) 2011-08-03 17:32:57 PDT
Ehsan, the test in this patch seems to fail because of other tests using mockObject.js. It seems that calling my test followed by toolkit/content/tests/browser/browser_save_resend_postdata.js or the latter followed by my test always creates a time out.

Here is the patch I pushed to try (the test has changed a bit):
https://hg.mozilla.org/try/rev/c56b80fa9d76

And the error:
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1312340808.1312345039.7982.gz#err0

By any chance, do you have any idea of what could be happening?
Comment 12 Mounir Lamouri (:mounir) 2011-08-09 16:39:28 PDT
Created attachment 551933 [details] [diff] [review]
Patch with updated tests
Comment 13 Mounir Lamouri (:mounir) 2011-08-10 18:01:01 PDT
So, I'm trying to understand what's happening with this test... I've tried to improve mockObjects.js by unregistering the current factory and re-register it correctly at the end and that helped to narrow down the issue.
This code fails with an exception thrown when .createInstance() is called because no factory is found:
(I believe the variable names are self-explanatory)
componentRegistrar.unregisterFactory(this._originalCID, this._originalFactory);
componentRegistrar.registerFactory(this._cid, "",
                                   this._contractID, this._mockFactory);
Components.classes[this._contractID].createInstance(Components.interfaces.nsIFilePicker);

It seems that nsComponentManagerImpl::RegisterFactory is correctly called and works so a factory is actually registered but when nsComponentManagerImpl::CreateInstance is called, there is indeed no factory but  these methods don't have the same CID in parameter. ::RegisterFactory has this._cid, as expected while ::CreateInstance is using the CID from the previous factory. I really don't understand why the previous CID is still around except if Components.classes[this._contractID] doesn't return the correct object and nsJSCID::mDetails hasn't been updated...
Comment 14 Mounir Lamouri (:mounir) 2011-08-11 09:34:49 PDT
Bug 678166 comes with the required fix to get the tests passing... Finally able to push! :)
Comment 15 Mounir Lamouri (:mounir) 2011-08-12 06:56:02 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/223d4f4bd252
Comment 16 christian 2011-10-31 16:43:44 PDT
I backed this out of mozilla-beta / Firefox8 due to bug 679961:

http://hg.mozilla.org/releases/mozilla-beta/rev/f8b0cc0c644a

We need to see if we can alter this a bit in bug 679961 (or get product to make an explicit decision)
Comment 17 Simona B [:simonab] 2011-11-03 08:00:16 PDT
Verified that this fix is backed out from Firefox 8 Beta 6 (build 2) using the test case attached in Comment 4. Tested on Windows XP, Windows 7, Mac OS X 10.6 and Ubuntu 11.10 - pop-ups appear when opening the save dialog.

Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0

Should this bug be reopened?
Comment 18 Mounir Lamouri (:mounir) 2011-11-03 09:29:39 PDT
(In reply to Simona B [QA] from comment #17)
> Should this bug be reopened?

The patch has been backed out from Aurora but is still applied on mozilla-central so the bug remains fixed.
Comment 19 Alex Keybl [:akeybl] 2011-12-01 14:31:17 PST
[Triage Comment]
Due to 679961, please back this bug out on aurora and beta, and also m-c if we don't think 679961 will be fixed in the short term.
Comment 20 Michael Lefevre 2011-12-07 07:38:11 PST
Reopening, as it seems (bug 679961 comment 15) this has now been backed out everywhere.
Comment 21 Andrew Overholt [:overholt] 2013-10-03 13:27:45 PDT
I can no longer reproduce this using nightly on Linux x86_64 as of today.

Note You need to log in before you can comment on or make changes to this bug.