Last Comment Bug 721322 - Garmin Communicator plugin stopped working for uploads on connect.garmin.com
: Garmin Communicator plugin stopped working for uploads on connect.garmin.com
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
http://connect.garmin.com
Depends on:
Blocks: 710941
  Show dependency treegraph
 
Reported: 2012-01-26 02:32 PST by Henrik Skupin (:whimboo)
Modified: 2012-02-10 12:34 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
verified


Attachments
Patch with tests (8.60 KB, patch)
2012-01-26 17:04 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Review

Description Henrik Skupin (:whimboo) 2012-01-26 02:32:51 PST
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) 2012-01-26 07:42:49 PST
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
Comment 2 Henrik Skupin (:whimboo) 2012-01-26 07:56:12 PST
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) 2012-01-26 07:57:17 PST
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] (remove +bmo to email) 2012-01-26 11:05:09 PST
Sigh, Linux haters gonna hate.  :-)  Thanks for noting that, probably saved me a bunch of time.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-26 11:37:11 PST
Okay, I think I have a guess about what's wrong here.  Testing that now.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-26 11:55:43 PST
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] (remove +bmo to email) 2012-01-26 17:04:17 PST
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.
Comment 8 Jason Orendorff [:jorendorff] 2012-01-27 17:34:17 PST
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] (remove +bmo to email) 2012-01-30 19:34:37 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5337afc5350
Comment 10 Ed Morley [:emorley] 2012-01-31 06:52:51 PST
https://hg.mozilla.org/mozilla-central/rev/a5337afc5350
Comment 11 Henrik Skupin (:whimboo) 2012-02-02 01:29:27 PST
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) 2012-02-03 13:06:10 PST
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!

Note You need to log in before you can comment on or make changes to this bug.