All users were logged out of Bugzilla on October 13th, 2018

download manager is empty

VERIFIED FIXED in mozilla1.9beta3

Status

()

--
blocker
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Peter6, Assigned: rflint)

Tracking

({regression})

Trunk
mozilla1.9beta3
regression
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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
Flags: blocking-firefox3?
If the bad patch can be discovered in the next three hours, I can back it out before nightlies.
Peter6, check the Error Console for me, would you? Anything there?
(Reporter)

Comment 3

11 years ago
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
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. :)
OS: Windows 2000 → All
Hardware: PC → All
Assignee: nobody → rflint
Target Milestone: --- → Firefox 3 M11
Created attachment 295481 [details] [diff] [review]
Patch

This fixes the compilation errors and removes other silly let uses (global, function block, etc).
Attachment #295481 - Flags: review?(dtownsend)
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.
Attachment #295481 - Flags: review?(dtownsend) → review+
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

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

verified
Status: RESOLVED → VERIFIED
I remember when let was the new var. :(
So, reading http://bugs.ecmascript.org/ticket/280, it seems to me that a lot of these let->var replacements were *not* needed...
(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
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Blocks: 408957

Comment 13

11 years ago
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

11 years ago
(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

11 years ago
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

11 years ago
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.
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. :)
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
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
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 295550 [details] [diff] [review]
Patch

Fine. Hooray for MQ.
Attachment #295481 - Attachment is obsolete: true
Attachment #295550 - Flags: review?(comrade693+bmo)

Comment 21

11 years ago
(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).
(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
(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 on attachment 295550 [details] [diff] [review]
Patch

r=sdwilsh
Attachment #295550 - Flags: review?(comrade693+bmo) → review+

Updated

11 years ago
Attachment #295550 - Flags: approval1.9?

Updated

11 years ago
Attachment #295550 - Flags: approval1.9? → approval1.9+
Created attachment 295657 [details] [diff] [review]
v1.0 test

here's a test
Attachment #295657 - Flags: review?(gavin.sharp)
Attachment #295657 - Flags: review?(gavin.sharp) → review+
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
Created attachment 296064 [details] [diff] [review]
Patch
Attachment #295550 - Attachment is obsolete: true
Attachment #296064 - Flags: review?(brendan)
Comment on attachment 296064 [details] [diff] [review]
Patch

r=sdwilsh
Attachment #296064 - Flags: review?(brendan)
Attachment #296064 - Flags: review+
Attachment #296064 - Flags: approval1.9?
Comment on attachment 296064 [details] [diff] [review]
Patch

Thanks, Ryan.

/be
Attachment #296064 - Flags: approval1.9? → approval1.9+
(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 :)
"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
Created attachment 296084 [details] [diff] [review]
Test addendum

This gets the tests passing on windows.
Attachment #296084 - Flags: review?(comrade693+bmo)
(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 on attachment 296084 [details] [diff] [review]
Test addendum

Land her :)
Attachment #296084 - Flags: review?(comrade693+bmo) → review+
/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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Flags: in-litmus- → in-litmus+
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
Status: RESOLVED → VERIFIED

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.