353560 - Unknown Content dialog not dismissed when filepicker opens

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060920 BonEcho/2.0

When clicking "Save" on the unknown content dialog the filepicker displays to let me choose a save location. The unknown content dialog's Save button becomes disabled but the dialog remains visible. It is possible to click the cancel button on the dialog before dismissing the file picker, although nothing seems to happen.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Screenshot

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

See bug 249143 for context. This is not a regression (exists in the 1.5.0.x branch).

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Also happens in the "normal" case where open and save are radio options.

Comment 4

[Asaf Romano (gone)]

Something really bad happens here, the "parent" window in promptForSaveToFile is the content window from which the download has been fired. It's expected to be the Unknown Content dialog...

Comment 5

[Asaf Romano (gone)]

Attachment: patch

So, here's what happens here:
1. saveToDisk calls promptForSaveToFile which opens a file picker attached to the content window from which the download has been fired.
2. the file picker blocks the onOK routine (which is fired on-dialog-accpet).
3. the dialog loses focus, and therefore the button is disabled.

The fix is to call saveToDisk only once the dialog is closed.

Comment 6

[Asaf Romano (gone)]

FWIW, I don't think should block either 2.0 or 1.5.0.8.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

I tend to agree; minor issue, not going to block the release at this point, and not sure we want to mess with this code at this point in the cycle. Sorry, Ben.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #7)
> I tend to agree; minor issue, not going to block the release at this point, and
> not sure we want to mess with this code at this point in the cycle. Sorry, Ben.
> 

Hah, no tears here, I promise :)

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Screenshot of multiple UC dialogs

Wow, look what else can happen. The fact that the UC dialog is not modal to the window means that you can switch back to the original link and re-open the UC dialog as many times as you want. It appears in the exact same place as the most recent UC dialog, so you have to move the window around to realize you've got more than one.

Asaf, your patch won't fix this additional complication, will it?

Comment 10

[Daniel Veditz [:dveditz]]

Not making 1.8.0.x without a landed, reviewed, tested patch.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 239449 [details] [diff] [review]
patch

The only thing I'm concerned about here is that this makes us call updateHelperAppPref() before saveToDisk() - are you sure that that's OK? It looks to me like it should be, since nsExternalAppHandler::SaveToDisk doesn't depend on any of the stuff updateHelperAppPref() does (and vice-versa) but I'm not that familiar with this code so I'd like to make sure.

Comment 12

[Asaf Romano (gone)]

(In reply to comment #9)

> Asaf, your patch won't fix this additional complication, will it?
> 

Please test in the next nightly build.

mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in 1.40

Comment 13

[Asaf Romano (gone)]

backed out due to bug 356178.

Comment 14

[Asaf Romano (gone)]

Attachment: Use the opener

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 244684 [details] [diff] [review]
Use the opener

r=me assuming this doesn't regress bug 356178.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(though I'm a little wary about relying on the fact that the .opener property can point to a closed window in the target="_blank" case, given bug 241972 and bug 353851. should be fine for now, I guess)

Comment 17

[Asaf Romano (gone)]

mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js 1.42

Comment 18

[Daniel Veditz [:dveditz]]

Is this patch safe for the 1.8 branch? Did we get answers about the regression questions?

Comment 19

[Asaf Romano (gone)]

I don't think it's worth the risk (when I nominated this bug i thought it was a regression).

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #18)
> Did we get answers about the regression questions?

Sorry, I dropped the ball. Not a regression.

Comment 21

[Rob Arnold [:robarnold]]

This bug did cause a regression. See bug 383812.