Open Bug 631304 Opened 11 years ago Updated 3 years ago

Unknown content type dialog sometimes show simple ui even it states to show the normal ui

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: u279076, Unassigned)

References

Details

(Whiteboard: [mozmill-test-blocked])

Attachments

(2 files, 1 obsolete file)

Attached image Screenshot
Currently, in downloadManager.downloadFileOfUnkownType() we only wait for the Download Manager dialog to open.  We don't verify the loading of its content.  As a result, we fail when an empty dialog appears.  While this happens rarely, it does happen (see screenshot).

NOTE: This came out of bug 631246.
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
With my fix for the mozmill internal window.loaded checks it should normally not happen. When we are searching for the window do we use handleWindow or simply a getMostRecentWindow? We have to wait for window.documentLoaded here.
We use getMostRecentWindow before calling handleWindow:

controller.waitForEval("subject.getMostRecentWindow('').document.documentElement.id == 'unknownContentType'", gTimeout, 100, mozmill.wm);
I did a test by dumping documentLoaded between the waitForEval and handleWindow.  In the case of the empty dialog, documentLoaded is still TRUE.
Also, adding a sleep(100) here does not prevent the empty dialog from occurring.
(In reply to comment #4)
> Also, adding a sleep(100) here does not prevent the empty dialog from
> occurring.

Bumping up the number to 1000 (a full second) seems to have prevented this dialog from happening.  It's far from ideal, but I'm fine with checking this in as a temporary workaround.

Henrik, what do you think?
(In reply to comment #5)
> (In reply to comment #4)
> > Also, adding a sleep(100) here does not prevent the empty dialog from
> > occurring.
> 
> Bumping up the number to 1000 (a full second) seems to have prevented this
> dialog from happening.  It's far from ideal, but I'm fine with checking this in
> as a temporary workaround.
> 
> Henrik, what do you think?

Ugh, scratch that...I did a second testrun of 100 and it happened this time...so after 147 testruns it happens once.  A sleep(1000) simply makes it happen far less often.
I'm wondering if we shouldn't bring in a developer who works on the code in this area. Is it possibly a Firefox bug?
(In reply to comment #7)
> I'm wondering if we shouldn't bring in a developer who works on the code in
> this area. Is it possibly a Firefox bug?

Sounds like. I think you can ask Shawn Wilsher, or check mxr for the xul/js file. There should be at least some names.
As per email, Shawn, if you have any information we can use to debug what might be happening here, please comment on this bug.

Thanks
If this is a core issue we should move it accordingly.

Shawn, does this dialog use any timer to lazy display the window content dependent on the MIME type?
(In reply to comment #10)
> Shawn, does this dialog use any timer to lazy display the window content
> dependent on the MIME type?
It doesn't use a timer, no.

I'm curious as to what you are testing with this though; it should already have coverage through automated tests.
(In reply to comment #11)
> I'm curious as to what you are testing with this though; it should already have
> coverage through automated tests.

At the moment our Mozmill tests do not support the save file dialog, because those are native. So using the unknown type dialog allows us to run tests for the download manager.
In mozilla-central, we have this chrome test that opens the dialog:
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul

Check to make sure you aren't doing something terribly different?
Looking at that test file, I'm wondering if we are triggering the simple UI in some cases:

63 let tests = [
64   { // This URL will trigger the simple UI, where only the Save an Cancel buttons are available
65     url: "http://mochi.test:8888/chrome/toolkit/mozapps/downloads/tests/chrome/unknownContentType_dialog_layout_data.pif",
66     elements: {
67       basicBox: { collapsed: false },
68       normalBox: { collapsed: true }
69     }
70   }
(In reply to comment #14)
> Looking at that test file, I'm wondering if we are triggering the simple UI in
> some cases:
That should give you some buttons still though.
(In reply to comment #15)
> (In reply to comment #14)
> > Looking at that test file, I'm wondering if we are triggering the simple UI in
> > some cases:
> That should give you some buttons still though.

As per the attached screenshot, we only get the SUBMIT and CANCEL buttons.
(In reply to comment #13)
> In mozilla-central, we have this chrome test that opens the dialog:
> https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul
> 
> Check to make sure you aren't doing something terribly different?

This test doesn't check the radio buttons. Where the code which decides if the simple UI has to be displayed? If we are triggering this behavior some times, it looks like a bug in that code.
From the XUL file it looks like the dialog has two properties, basicBox and normalBox which are true,false respectively for the full UI.  I'm wonder if we should/could check for this and enforce it in our tests?
(In reply to comment #18)
> From the XUL file it looks like the dialog has two properties, basicBox and
> normalBox which are true,false respectively for the full UI.  I'm wonder if we
> should/could check for this and enforce it in our tests?

Sorry, that should be:
basicBox.collapsed = true and
normalBox.collapsed = false
We always should get the same type of dialog. There shouldn't be a flip to the basic box under unknown situations.
(In reply to comment #20)
> We always should get the same type of dialog. There shouldn't be a flip to the
> basic box under unknown situations.

Without discounting that, we should at least extend DownloadsAPI to check that state so we can report "full UI" vs "simple UI" to brasstacks.

I agree that we should always be getting the same dialog which should also be taken care of on this bug.
That should be a fairly easy patch. Do you mind working on it? Beside that we should file a separate bug for the dialog itself.
(In reply to comment #22)
> That should be a fairly easy patch. Do you mind working on it? Beside that we
> should file a separate bug for the dialog itself.

Yup, I can work on that patch.  I'll file a new bug for the simple-UI issue.
(In reply to comment #10)
> Shawn, does this dialog use any timer to lazy display the window content
> dependent on the MIME type?
I forgot that the window is actually opened by a timer:
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#175

The code to initialize the dialog is here:
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#404
That would mean that shouldntRememberChoice and openWithDefaultOK() are the only conditions which let us select the type of box here. We could dump both values of the above items to check which one is causing this problem.

I think we should keep this bug for the helper dialog and file a new one for updating our shared module.
(In reply to comment #25)
> I think we should keep this bug for the helper dialog and file a new one for
> updating our shared module.

I'm confused about what you want here now...
Lets work out the simple update for the download.js module on bug 631246. This bug already has a lot of valuable information which will help to find the reason for that strange behavior.
Assignee: anthony.s.hughes → nobody
Component: Mozmill Tests → Download Manager
Product: Mozilla QA → Toolkit
QA Contact: mozmill-tests → download.manager
Attached file testDownloadStates-simplified.js (obsolete) —
Here is a simplified version of testDownloadStates.  Henrik, please test
locally and you will see what I mean.

You should notice that under all conditions (whether we see the Normal UI or
the Simple UI), normalBox.collapsed is always FALSE and basicBox.collapsed is
always TRUE.
Here is a new version which dumps shouldnRememberChoice and openWithDefaultOK.

Here is a sample output:
 *** TESTRUN 5 ***
 *** MIME-Type: application/x-bzip2
 *** isExecutable: false
 *** shouldntRememberChoice: false
 *** openWithDefaultOK: true
 *** EXPECT normalBox.collapsed=FALSE, basicBox.collapsed=TRUE
 *** normalBox.collapsed: false
 *** basicBox.collapsed: true


 *** TESTRUN 6 ***
 *** MIME-Type: application/x-bzip2
 *** isExecutable: false
 *** shouldntRememberChoice: false
 *** openWithDefaultOK: true
 *** EXPECT normalBox.collapsed=FALSE, basicBox.collapsed=TRUE
 *** normalBox.collapsed: false
 *** basicBox.collapsed: true

Testrun 5 we get the full UI.
Testrun 6 we get the simple UI.
Both datasets are completely identical.
Attachment #510736 - Attachment is obsolete: true
Attachment #510788 - Flags: feedback?(hskupin)
Comment on attachment 510788 [details]
testDownloadStates-simplified.js v2

Looks fine. Now we only need one instance of a test failure. It would probably worth to run it in a vm where it always happened.
Attachment #510788 - Flags: feedback?(hskupin) → feedback+
(In reply to comment #30)
> Looks fine. Now we only need one instance of a test failure. It would probably
> worth to run it in a vm where it always happened.

Scratch that. Missed to look at the former comments.
Summary: Improve DownloadAPI to check for content of Save File dialog → Unknown content type dialog sometimes show simple ui even it states to show the normal ui
Whiteboard: [mozmill-test-blocked]
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#500

I'm wondering if this "hack" is failing to work in some cases.  Shawn, do you have any advice on how I can debug this?
Bug 614557 fixed Venkman today. So if you build the extension on your own, you could use it with mozmill, in case we can reproduce it manually. Or another idea, we could create a mozmill test like the testcase on this bug, and check for the simple/normal ui and the radio buttons. If the radio button throw an error lets catch it and call a long sleep so we would have enough time to manually investigate this problem and set breakpoints.
(In reply to comment #32)
> I'm wondering if this "hack" is failing to work in some cases.  Shawn, do you
> have any advice on how I can debug this?
I don't even understand why we'd need that.  That code has been there since day one, so there's no bug to look at as to why it was added either :/
I have uploaded a temporary version of Venkman with the fix on bug 614557 here:
http://people.mozilla.com/~hskupin/downloads/

Lets see if I can come closer to the problem.
Is this bug still worth working on or should we just close it?
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.