410894 - download manager is empty

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008010423 Minefield/3.0b3pre ID:2008010423

repro:
open FF
open DM

result:
it's empty

expected:
a number of files that are supposed to be there

regressionrange:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1199496660&maxdate=1199497739

Comment 1

[Reed Loden [:reed]]

If the bad patch can be discovered in the next three hours, I can back it out before nightlies.

Comment 2

[Reed Loden [:reed]]

Peter6, check the Error Console for me, would you? Anything there?

Comment 3

[Peter van der Woude [:Peter6]]

on startup of DM:
Error: let declaration not directly within block
Source file: chrome://mozapps/content/downloads/downloads.js
Line: 411, Column: 8
Source code:
        let id = aSubject.QueryInterface(Ci.nsISupportsPRUint32); 
Error: XPCOMUtils is not defined
Source file: chrome://mozapps/content/downloads/DownloadProgressListener.js
Line: 16
Error: Startup is not defined
Source file: chrome://mozapps/content/downloads/downloads.xul
Line: 1
Error: gDownloadViewController is not defined
Source file: chrome://mozapps/content/downloads/downloads.xul
Line: 1

on shutdown of DM
Error: Shutdown is not defined
Source file: chrome://mozapps/content/downloads/downloads.xul
Line: 1

Comment 4

[Reed Loden [:reed]]

Bug 408957 caused this, but it actually just uncovered a problem with the Download Manager code, so the fix will be to patch the dlmgr to do the right thing. :)

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

This fixes the compilation errors and removes other silly let uses (global, function block, etc).

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 295481 [details] [diff] [review]
Patch

r=me to get the bustage fixed. Probably want to spin it past sdwilsh or someone after the fact though.

Comment 7

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/toolkit/mozapps/downloads/content/downloads.js 1.128

Cashed in a little cred banking on the that this will likely be granted blocking-fx3.

Comment 8

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008010502 Minefield/3.0b3pre ID:2008010502

verified

Comment 9

[Mike Shaver (:shaver emeritus)]

I remember when let was the new var. :(

Comment 10

[Shawn Wilsher :sdwilsh]

So, reading http://bugs.ecmascript.org/ticket/280, it seems to me that a lot of these let->var replacements were *not* needed...

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Created an attachment (id=295481) [details]
> Patch
> 
> This fixes the compilation errors and removes other silly let uses (global,
> function block, etc).

Why remove all the well-formed let uses? They are not silly. Please reconsider.

/be

Comment 12

[Brendan Eich [:brendan]]

Reopening so the set of let declarations in the previous download.js revision that are now flagged as syntax errors can be identified. Just changing let to var may be covering up a bug in the JS engine. Don't risk that by making such overlarge changes!

/be

Comment 13

[Eddie]

Just an FYI, this seems to also affect DownThemAll extension (both the 1.0b2 and nightlies).  Using DTA, after updating to today's (20080105) nightly release of Minefield, DTA queue was empty and nothing could be put into it, it displayed nothing to download.  Backed out todays build to the 2008010405 build and DTA works normally.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010405 Minefield/3.0b3pre ID:2008010405

Adblock Plus 0.7.5.3+.2007100317
DictionarySearch 2.0.2
DOM Inspector 1.9b3pre
DownloadHelper 2.5.3
DownThemAll! 1.0b2
Flashblock 1.5.5
Mouse Gestures 1.5.2
Nightly Tester Tools 1.3b3
Update Channel Selector 1.0.2

Comment 14

[Ed Lee :Mardak]

(In reply to comment #10)
> it seems to me that a lot of these let->var replacements were *not* needed...
All that work converting/adding in lets instead of vars and all that blame...

The line in question looks like the last line of..

let a = {
  b: function(c) {
    switch (c) {
      case "":
        if (d) { 1 }
        let e = 2;
}}}

So the the problem line "let e" seems to be correctly used at a block... ?

I thought there was already block level requirements for lets.. that's why I coded this to have a block let instead of a let expression for the first let.

let (sb = document.getElementById("downloadStrings")) {
  let getStr = function(string) sb.getString(string);
  for (let [name, value] in Iterator(gStr))
    gStr[name] = typeof value == "string" ? getStr(value) : value.map(getStr);
}

Comment 15

[Ed Lee :Mardak]

js> switch (1) { case 2: let a = 3; }
typein:9: SyntaxError: let declaration not directly within block:
typein:9: switch (1) { case 2: let a = 3; }
typein:9: .....................^

switch (1) { case 2: { let a = 3; } }
That works though.

Comment 16

[Ed Lee :Mardak]

Seems like there was some discussion about this for ES4..

but switch bodies are not blocks, grammatically. They look like blocks, however, and I think they should scope let as other blocks do.
— Brendan Eich 2006/09/07 16:27
http://wiki.ecmascript.org/doku.php?id=proposals:block_expressions&s=switch

But I suppose here, all we have is a statementlist and not a block.

Comment 17

[Ryan Flint [:rflint] (ping via IRC for reviews)]

When writing this patch, I fixed the syntax errors first and made sure that JS didn't balk at anything else before writing the rest of it. I saw nothing wrong on the JS engine side of things with any of the uses flagged as syntax errors.

If reversion of the unnecessary changes is desired, I'll gladly do so in a follow up in exchange for a unit test that verifies that the DM UI is actually functional. :)

Comment 18

[Brendan Eich [:brendan]]

Yeah, switch in C has a body block -- and labels can go anywhere! (See, e.g., Duff's Device). switch in JS is like Java's switch, more structured (but still with fall-through of course). No let decl in case statement list -- it's not an implicit block. Perhaps it should be -- Mardak, are you willing to file a trac ticket at http://bugs.ecmascript.org?

The rest of the let->var changes should be undone.

(Digression: why not put both let vars in the head of the let block in:

let (sb = document.getElementById("downloadStrings")) {
  let getStr = function(string) sb.getString(string);
  for (let [name, value] in Iterator(gStr))
    ...
}

instead of putting getStr in the block? No harm, just wanted to ask.)

/be

Comment 19

[Brendan Eich [:brendan]]

Ryan: I'm not bargaining with you -- that overlarge change was unnecessary and obscured the issue (and possibly a JS engine bug). It should be undone, and the minimal fix made. This bug was not fixed properly, so I'm reopening it. Don't close it again without a new patch.

/be

Comment 20

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Fine. Hooray for MQ.

Comment 21

[Ed Lee :Mardak]

(In reply to comment #18)
> No let decl in case statement list -- it's not an
> implicit block. Perhaps it should be -- Mardak, are you willing to file a trac
> ticket at http://bugs.ecmascript.org?
Filed ticket #347. http://bugs.ecmascript.org/ticket/347

Ryan: Just curious, how did you choose which lets to convert into vars? Just the plain let statements like "let a = b;" and skipped the let blocks? I noticed in the latest patch, there were some existing "let (block) {}"

(In reply to comment #18)
> (Digression: why not put both let vars in the head of the let block in:
> let (sb = document.getElementById("downloadStrings")) {
>   let getStr = function(string) sb.getString(string);
The outer let defines sb which the lambda uses (so no defining both at once).

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> Filed ticket #347. http://bugs.ecmascript.org/ticket/347

Thanks.

> (In reply to comment #18)
> > (Digression: why not put both let vars in the head of the let block in:
> > let (sb = document.getElementById("downloadStrings")) {
> >   let getStr = function(string) sb.getString(string);
> The outer let defines sb which the lambda uses (so no defining both at once).

Argh, how did I miss that? What I'm getting at is the trade-offs you used when deciding between let blocks and blocks containing let declarations. I'm guessing you used a let decl for getStr to avoid overindenting the rest of the let block body? Ecma TG1 is looking for more feedback on let from real-world users, so feel free to mail me.

/be

Comment 23

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #21)
> Ryan: Just curious, how did you choose which lets to convert into vars? Just
> the plain let statements like "let a = b;" and skipped the let blocks? I
> noticed in the latest patch, there were some existing "let (block) {}"
The declaration of |block| is occurring within the block created by that expression, so it doesn't face the same issue in a case body as the plain statement.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 295550 [details] [diff] [review]
Patch

r=sdwilsh

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: v1.0 test

here's a test

Comment 26

[Brendan Eich [:brendan]]

I just committed the fix for bug 411279, so this bug should be patched by rolling back all changes to change any let to var. Can someone please do that ASAP? I'll review if needed.

/be

Comment 27

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Comment 28

[Shawn Wilsher :sdwilsh]

Comment on attachment 296064 [details] [diff] [review]
Patch

r=sdwilsh

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 296064 [details] [diff] [review]
Patch

Thanks, Ryan.

/be

Comment 30

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

(In reply to comment #26)
> I just committed the fix for bug 411279, so this bug should be patched by
> rolling back all changes to change any let to var.

Why change existing vars to let in the cases where there are no benefits to doing so (e.g. the global variables in the first hunk of bug 296064)? I agree that they probably shouldn't have been changed in this bug in the first place (although had I reviewed the patch that added them I would have suggested just using "var" anyways), but I don't really see the point of changing them back and introducing another round of churn.

I'm not opposed to using "let" in new code where its possible to benefit from the more logical scoping, but I also don't think we should be actively replacing "var" with "let" globally just because it's new and hip :)

Comment 31

[Brendan Eich [:brendan]]

"Another round of churn" is more apt for what would happen if the first patch in this bug and the latest did not exactly cancel each other out. We should not be changing confounding variables, or altering Mardak's code, without good reason!

Also, let is better than var at top level, because lexically scoped instead of a property of the mutable variable object (the global object), in ES4 as specified. The top-level scope for let is an implicit block that is not affected by costly global object resolve, get, and set hooks, and not visible to code in the current or other windows that can access var bindings as properties of the global object.

The bug 346749 in SpiderMonkey is to-be-fixed, not a reason to prefer var at top level. Let ideally is better than var (faster, cheaper in space if you don't eval or otherwise capture the scope chain). SpiderMonkey will approach this ideal in the next few months, so code shouldn't be churned to work around current costs.

/be

Comment 32

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Test addendum

This gets the tests passing on windows.

Comment 33

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

(In reply to comment #31)
> "Another round of churn" is more apt for what would happen if the first patch
> in this bug and the latest did not exactly cancel each other out.

Fair enough.

> Also, let is better than var at top level, because lexically scoped instead of
> The top-level scope for let is an implicit block that is not affected by
> costly global object resolve, get, and set hooks, and not visible to code in
> the current or other windows that can access var bindings as properties of the
> global object.

Thanks, I wasn't aware of those benefits.

Comment 34

[Shawn Wilsher :sdwilsh]

Comment on attachment 296084 [details] [diff] [review]
Test addendum

Land her :)

Comment 35

[Ryan Flint [:rflint] (ping via IRC for reviews)]

/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in 1.3
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js 1.2
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js 1.130

Comment 36

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008010904 Minefield/3.0b3pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008010904 Minefield/3.0b3pre

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010904 Minefield/3.0b3pre