Garmin Communicator plugin stopped working for uploads on connect.garmin.com

VERIFIED FIXED in Firefox 12

Status

()

Core
JavaScript Engine
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: whimboo, Assigned: Waldo)

Tracking

({regression})

12 Branch
mozilla12
regression
Points:
---

Firefox Tracking Flags

(firefox12- verified)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a1) Gecko/20120118 Firefox/12.0a1

Between the Nightly builds 2012010503 an 2012010603 the plugin from Garmin stopped working at the upload page on http://connect.garmin.com due to the following error:

HtmlTagNotFoundException: Plug-In HTML tag not found.

WORKS: http://hg.mozilla.org/mozilla-central/rev/200a8d1fb452
FAILS: http://hg.mozilla.org/mozilla-central/rev/10894668e37f

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=200a8d1fb452&tochange=10894668e37f

I will start a bisect now to find the regressing bug.
(Reporter)

Comment 1

6 years ago
The first bad revision is:
changeset:   83719:377a1042074e
user:        Jeff Walden <jwalden@mit.edu>
date:        Wed Dec 14 20:08:02 2011 -0500
summary:     Bug 710941 - Simplify |a.<here>| parsing by having it not reuse primaryExpr parsing.  r=jorendorff
Assignee: nobody → general
Blocks: 710941
status-firefox12: --- → affected
tracking-firefox12: --- → ?
Component: Plug-ins → JavaScript Engine
Keywords: testcase-wanted
OS: Mac OS X → All
QA Contact: plugins → general
(Reporter)

Comment 2

6 years ago
There is no easy way to create a simplified testcase because saving to disk makes the symptoms go away. 

Steps:
1. Create an account at  http://connect.garmin.com
2. Open the upload page: http://connect.garmin.com/page/transfer/upload.faces

See the given failure above the plugin container.
(Reporter)

Comment 3

6 years ago
Oh, and keep in mind that the plugin is only supported on Windows and OS X. So you probably don't see that on Linux at all.
Sigh, Linux haters gonna hate.  :-)  Thanks for noting that, probably saved me a bunch of time.
Assignee: general → jwalden+bmo
Okay, I think I have a guess about what's wrong here.  Testing that now.
This is probably the reason for the failure:

  var obj =
    {
      test: function()
      {
        return obj.test.arguments !== null;
      }
    };
  assertEq(obj.test(), true);

This passes in the change before, fails in that change.  And it'd match the look of the code causing a JS error to show up in the console.

Should be an easy fix, looks like refactoring got rid of a tc->noteArgumentsUse().
Created attachment 592003 [details] [diff] [review]
Patch with tests

Well, maybe a little more involved.  And I stumbled across a bug with generator expressions in the process.  But still, not too bad.

Note that we want to land this before the merge date so the regression (thus far affecting only m-c) doesn't propagate into aurora.  So a little care about getting this reviewed sooner rather than later would be appreciated.
Attachment #592003 - Flags: review?(jorendorff)
Comment on attachment 592003 [details] [diff] [review]
Patch with tests

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

r=me with one fix:

::: js/src/frontend/Parser.cpp
@@ +6637,4 @@
>           * a reference of the form foo.arguments, which ancient code may
>           * still use instead of arguments (more hate).
>           */
> +        MOZ_ASSERT(!afterDot, "memberExpr now handles the afterDot case directly");

Believe it or not, this can happen:

  function f(){x..arguments}

Add the test please. That is the only way; the parameter here could be renamed to afterDblDot (while you're at it you could make its type bool), and this noteArgumentsNameUse(node) call can be under 'if (!afterDblDot)'.

The part of the comment talking about "foo.arguments" should be moved to memberExpr().

The enclosing block has a multiline if-condition, so while you're in here you could move the curly brace to its own line.

While it's on your mind, would you maybe look at the other places where we call countArgumentsUse()? I noticed that we reject this:

  ((let (arguments) 0) for (i in x))

even though the 'arguments' there definitely isn't a magic one. I don't care too much about this but it would be nice if there were actual rules of some kind. Eh.
Attachment #592003 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5337afc5350
Status: NEW → ASSIGNED
Keywords: testcase-wanted
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/a5337afc5350
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

6 years ago
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0a1) Gecko/20120201 Firefox/13.0a1

Version 13.0a1 is the next available nightly to test with. It should have been made it into 12.0a2 which I will check later today when the Aurora build is available.
Status: RESOLVED → VERIFIED

Updated

6 years ago
status-firefox12: affected → ---
(Reporter)

Comment 12

6 years ago
Also verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a2) Gecko/20120203 Firefox/12.0a2. That means we got the fix into 12.0 right in time. Thanks Jeff!
status-firefox12: --- → verified

Updated

5 years ago
tracking-firefox12: ? → -
You need to log in before you can comment on or make changes to this bug.