806743 - See if toolkit/components/places/tests/unit/test_download_history.js makes sense in the new PB world

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Comment 1

[Josh Matthews [:jdm]]

I'm modifying the test in bug 723005 to pass.

Comment 2

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #1)
> I'm modifying the test in bug 723005 to pass.

Hmm, if I'm reading attachment 676400 [details] [diff] [review] correctly, that just removes the test, right?

Comment 3

[(no longer active)]

Josh: ping?

Comment 4

[Josh Matthews [:jdm]]

Yes.

Comment 5

[(no longer active)]

So should we close this bug then?

Comment 6

[Josh Matthews [:jdm]]

It would probably be good to write a browser-chrome test that initiated a download and checked that it didn't end up in the history and that we didn't receive a link visited notification for it.

Comment 7

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Created a browser-chrome test as suggest in comment 6

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 693586 [details] [diff] [review]
Patch v1

Review of attachment 693586 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! Thanks!

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 693586 [details] [diff] [review]
Patch v1

Although the commit message could be more informative :)

Comment 10

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Updated commit message

Comment 11

[Josh Matthews [:jdm]]

Please run this through try first. This may break on mobile.

Comment 12

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=099eb4f32626

Comment 13

[Andres Hernandez [:andreshm]]

After looking at the logs. The test finished without errors, but after that there is an alert or the window loses focus, any idea what can it be?

INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | finished in 10475ms
TEST-INFO | checking window state
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | Found an unexpected alert:alert at the end of test run

INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | finished in 1003ms
TEST-INFO | checking window state
TEST-INFO | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | must wait for focus

Comment 14

[Josh Matthews [:jdm]]

Hmm, I'm not sure about the alert problem. However, it occurs to me that this test isn't actually testing what I was thinking it was - instead of setting up the save and download process manually, I think it would be a more useful test to initiate a download from a link on in a private window, wait until it's complete and ensure that there was no link visited notification and no record in the history.

Comment 15

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Updated patch to initialize download from a link.

Comment 16

[Josh Matthews [:jdm]]

Comment on attachment 696075 [details] [diff] [review]
Patch v3

Review of attachment 696075 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Sorry about the lag on my end.

Comment 17

[(no longer active)]

So is this ready to land?

Comment 18

[Josh Matthews [:jdm]]

Let's put it through try first.

https://tbpl.mozilla.org/?tree=Try&rev=e48396fdf052

Comment 19

[Andres Hernandez [:andreshm]]

The test finished without errors, but after the test finishes there is a timeout, any idea?

INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | finished in 2157ms
TEST-INFO | checking window state
TEST-INFO | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | must wait for focus
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | application timed out after 330 seconds with no output

Comment 20

[Josh Matthews [:jdm]]

Maybe something in

>   2.123 +      if (aDownload.targetFile.exists()) {
>   2.124 +        aDownload.targetFile.remove(false);
>   2.125 +      }

is throwing?

Comment 21

[Andres Hernandez [:andreshm]]

I tried using a try/catch block, but still it fails. But I'm sure now that is failing only on Windows. See: https://tbpl.mozilla.org/?tree=Try&rev=1d5c5394e902
I'll keep researching.

Comment 22

[(no longer active)]

Any updates on this?

Comment 23

[(no longer active)]

Over in bug 817477, I'm preparing to remove support for global private browsing altogether.  This is one of the last remaining pieces that I would like us to fix before that bug lands...

Comment 24

[Raymond Lee [:raymondlee]]

> >   2.123 +      if (aDownload.targetFile.exists()) {
> >   2.124 +        aDownload.targetFile.remove(false);
> >   2.125 +      }

I have investigated this and this part of code doesn't throw any errors as Andres mentioned in comment 21.  I suspect that when the targetFile.remove(false) is executed,  another UI (alert) appears so the last browser window loses the focus on Win.  I have tried to invoke window.focus() in registerCleanupFunction() but without luck.

Any thoughts?

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 696075 [details] [diff] [review]
Patch v3

>+        // Wait for acceptDialog to execute.
>+        function accept() {
>+          if (dialog.acceptDialog) {
>+            dialog.acceptDialog();
>+            executeSoon(accept);
>+          }
>+        }
>+        executeSoon(accept);

This doesn't look right to me. Shouldn't the executeSoon be outside of the if block, to ensure we wait until the acceptDialog method is available? Otherwise we just attempt to run the code over and over.

Comment 26

[Raymond Lee [:raymondlee]]

(In reply to Josh Matthews [:jdm] from comment #25)
> Comment on attachment 696075 [details] [diff] [review]
> Patch v3
> 
> >+        // Wait for acceptDialog to execute.
> >+        function accept() {
> >+          if (dialog.acceptDialog) {
> >+            dialog.acceptDialog();
> >+            executeSoon(accept);
> >+          }
> >+        }
> >+        executeSoon(accept);
> 
> This doesn't look right to me. Shouldn't the executeSoon be outside of the
> if block, to ensure we wait until the acceptDialog method is available?
> Otherwise we just attempt to run the code over and over.

Let me rewrite that part. ;-)

Comment 27

[Raymond Lee [:raymondlee]]

Attachment: v4

I have updated the patch based on comment 25

However, still getting issues on windows.
https://tbpl.mozilla.org/?rev=86bb39abbaa8&tree=Try

Any suggestions how to fix that?

Comment 28

[Josh Matthews [:jdm]]

Aha! It's the completed downloads alert - on OS X and Linux, there's a platform system that handles it. On Windows, we pop open http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/resources/content/alert.xul, which is of type alert:alert (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsAlertsService.cpp#116, for reference). You should set browser.download.manager.showAlertOnComplete to false when running this test, and clear it when cleaning up.

Comment 29

[Raymond Lee [:raymondlee]]

Attachment: v5

Thanks for the tip.

Pushed to try and hope it passes Try.
https://tbpl.mozilla.org/?tree=Try&rev=3f0a313a3828

Comment 30

[Raymond Lee [:raymondlee]]

Attachment: v6

The last patch still fails on Windows.

I have updated it again and it passes try.
https://tbpl.mozilla.org/?tree=Try&rev=9ce3e88e795c

Comment 31

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 32

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddedb68b1cc6

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ddedb68b1cc6