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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The first bad revision is:
changeset:   83719:377a1042074e
user:        Jeff Walden <[e-mail]>
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

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 4

[Jeff Walden [:Waldo]]

Sigh, Linux haters gonna hate.  :-)  Thanks for noting that, probably saved me a bunch of time.

Comment 5

[Jeff Walden [:Waldo]]

Okay, I think I have a guess about what's wrong here.  Testing that now.

Comment 6

[Jeff Walden [:Waldo]]

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().

Comment 7

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 8

[Jason Orendorff [: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.

Comment 9

[Jeff Walden [:Waldo]]

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

Comment 10

[Ed Morley [:emorley]]

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

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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!

Comment 13

[Paul Anderson]

Please tell me all the possible ways to update my Garmin GPS or should i go to https://www.garmingpsmapupdate.co/