463491 - When save dialog is open in pop up blocker does not work at all

Status: REOPENED

(Core :: DOM: Core & HTML, defect)

Description

[Amoor Avakian]

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 [He/Him]]

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 [He/Him]]

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]

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

Attachment: testcase

Comment 5

[Mounir Lamouri (:mounir)]

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

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

Attachment: Patch v1

Comment 8

[Mounir Lamouri (:mounir)]

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

Jst, any update regarding the review?

Comment 10

[Johnny Stenback (:jst)]

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

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

Attachment: Patch with updated tests

Comment 13

[Mounir Lamouri (:mounir)]

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

Bug 678166 comes with the required fix to get the tests passing... Finally able to push! :)

Comment 15

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/223d4f4bd252

Comment 16

[LegNeato]

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 Badau, Desktop QA]

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

(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]]

[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]

Reopening, as it seems (bug 679961 comment 15) this has now been backed out everywhere.

Comment 21

[Andrew Overholt [:overholt]]

I can no longer reproduce this using nightly on Linux x86_64 as of today.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 23

[Kimberly Dorjean]

Attachment: same problem

Thanks from gresrimismi1978.