786114 - Firefox 17 doesn't display graphs anymore on Khan Academy (recent regression)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Loic]

Reporter: http://forums.mozillazine.org/viewtopic.php?p=12231281#p12231281

Open http://www.khanacademy.org/math/algebra/algebra-functions/e/graphing_points_2
Graph is missing.

m-c
good=2012-08-26
bad=2012-08-27
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b3cce81fef1a&tochange=8af6a22827ec

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/0a9e931cdcf3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120826192258
Bad:
http://hg.mozilla.org/mozilla-central/rev/8af6a22827ec
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120826222258
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0a9e931cdcf3&tochange=8af6a22827ec



Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/74666a1eb791
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120826130359
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d178a49c979c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120826160958
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=74666a1eb791&tochange=d178a49c979c


Suspected: Bug 784608

Comment 2

[Alice0775 White]

First bad :
660d18ddffcd	Nicholas Nethercote — Bug 784608 (part 7) - Change the form and meaning of ParseContext::innermostWith, and do follow-up simplifications. r=luke.

Comment 3

[Nicholas Nethercote [inactive]]

In http://hg.mozilla.org/integration/mozilla-inbound/rev/660d18ddffcd it's the change to needsImplicitThis() that's the problem.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Fix handling of nested functions that need implicit |this| within eval().

Nested functions that needs implicit |this| within an eval() weren't being
handled correctly.  This patch adds handling for that -- it's the third case
in FunctionBox(), which was missing.  The patch also adds a bunch of
comments to FunctionBox() to better explain what's going on.  And it adds a
test.

ps: The JS code at www.khanacademy.org is an abomination.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Backout d178a49c979c (bug 785608 part 8) for regressing |with| handling.

Let's just back out the regressing patch on Aurora.  This is the patch that
followed it, and so must be backed out as well.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Backout 660d18ddffcd (bug 785608 part 7) for regressing |with| handling.

This is the backout of the regressing patch.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 656737 [details] [diff] [review]
Backout d178a49c979c (bug 785608 part 8) for regressing |with| handling.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 784608
User impact if declined: some incorrect JS behaviour
Testing completed (on m-c, etc.): just backs out the regressing patch
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 656739 [details] [diff] [review]
Backout 660d18ddffcd (bug 785608 part 7) for regressing |with| handling.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 784608
User impact if declined: some incorrect JS behaviour
Testing completed (on m-c, etc.): just backs out the regressing patch
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 656721 [details] [diff] [review]
Fix handling of nested functions that need implicit |this| within eval().

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

Nice testcase and comments.  'with' is some piece of work.

::: js/src/frontend/Parser.h
@@ +188,5 @@
>      bool            parsingForInit:1;   /* true while parsing init expr of for;
>                                             exclude 'in' */
>      bool            parsingWith:1;  /* true while we are within a
>                                         with-statement or E4X filter-expression
> +                                       in the current ParseContext chain 

trailing whitespace

Comment 10

[Nicholas Nethercote [inactive]]

> 'with' is some piece of work.

We should, like, totally ban it in strict mode or something.

Comment 11

[Nicholas Nethercote [inactive]]

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/325b276940b5

Still need approval to back the old patches out from Aurora.

Comment 12

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/325b276940b5

Comment 13

[Nicholas Nethercote [inactive]]

The whiteboard says "[leave open]" -- we need to land the other patches on Aurora.

Comment 14

[Philipp von Weitershausen [:philikon]]

Sorry, must've missed that.

That said, that's not how we traditionally have dealt with back ports. AFAIK, we resolve a bug as soon as it hits mozilla-central and use the status-firefoxN flags to mark whether it's been fixed in Aurora/Beta/Release.

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4b0c5e6f8db
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ab679cebfe4

Comment 16

[chaosfreedom1220]

Site is not displaying correctly again. Practice work is broken, and the google map refuses to be usable. 

Nightly x64, 10/6/2012 build.

Comment 17

[Scoobidiver (away)]

chaosfreedom1220, this bug is fixed. If you are sure it's a Firefox problem (happens with a new profile), file a new bug.

Comment 18

[Loic]

(In reply to chaosfreedom1220 from comment #16)
> Site is not displaying correctly again. Practice work is broken, and the
> google map refuses to be usable. 
> 
> Nightly x64, 10/6/2012 build.

See bug 798834.

Comment 19

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0 beta 1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 beta 1

Verified on Firefox 17 beta 1 that the graphing math problems on khan academy are properly displayed on Windows 7 both  32 bit version and 64 bit version (verified also that other graphics from the left menu are also displayed correctly).

Comment 20

[Paul Silaghi, QA [:pauly]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0b1