Closed Bug 786114 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

17 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox17 + verified
firefox18 + verified

People

(Reporter: epinal99-bugzilla2, Assigned: njn)

References

Details

(Keywords: regression)

Attachments

(3 files)

Keywords: regression
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
Assignee: nobody → general
Blocks: 784608
Component: General → JavaScript Engine
First bad :
660d18ddffcd	Nicholas Nethercote — Bug 784608 (part 7) - Change the form and meaning of ParseContext::innermostWith, and do follow-up simplifications. r=luke.
Assignee: general → n.nethercote
In http://hg.mozilla.org/integration/mozilla-inbound/rev/660d18ddffcd it's the change to needsImplicitThis() that's the problem.
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.
Attachment #656721 - Flags: review?(luke)
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.
Attachment #656737 - Flags: review?(luke)
This is the backout of the regressing patch.
Attachment #656739 - Flags: review?(luke)
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
Attachment #656737 - Flags: approval-mozilla-aurora?
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
Attachment #656739 - Flags: approval-mozilla-aurora?
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
Attachment #656721 - Flags: review?(luke) → review+
Attachment #656737 - Flags: review?(luke) → review+
Attachment #656739 - Flags: review?(luke) → review+
> 'with' is some piece of work.

We should, like, totally ban it in strict mode or something.
Landed on inbound:

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

Still need approval to back the old patches out from Aurora.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/325b276940b5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
The whiteboard says "[leave open]" -- we need to land the other patches on Aurora.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #656737 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656739 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b4b0c5e6f8db
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ab679cebfe4
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Keywords: verifyme
Site is not displaying correctly again. Practice work is broken, and the google map refuses to be usable. 

Nightly x64, 10/6/2012 build.
chaosfreedom1220, this bug is fixed. If you are sure it's a Firefox problem (happens with a new profile), file a new bug.
(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.
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).
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0b1
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.