Bug 19830 - REGRESSION (r34883): Google Reader doesn't show up feed list on sidebar
Summary: REGRESSION (r34883): Google Reader doesn't show up feed list on sidebar
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-30 12:38 PDT by Ismail Donmez
Modified: 2008-06-30 18:57 PDT (History)
1 user (show)

See Also:


Attachments
Screenshot showing the issue. (140.56 KB, image/png)
2008-06-30 12:41 PDT, Ismail Donmez
no flags Details
Proposed patch (1.73 KB, patch)
2008-06-30 18:02 PDT, Cameron Zwarich (cpst)
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ismail Donmez 2008-06-30 12:38:26 PDT
This is a regression introduced in last 6 hours.

How to reproduce:

- Go to http://reader.google.com
- Make sure Show: Updated is selected

Now the new feeds entries are visible on the right side but the sidebar shows no feed title/count.
Comment 1 Ismail Donmez 2008-06-30 12:41:24 PDT
Created attachment 22010 [details]
Screenshot showing the issue.
Comment 2 Cameron Zwarich (cpst) 2008-06-30 13:46:56 PDT
I can confirm that this regresses with r34883.
Comment 3 Cameron Zwarich (cpst) 2008-06-30 16:31:31 PDT
This only happens when I turn on the optimization for the emitJumpIfFalse from ConditionalNode, but I am not sure why. The optimization should be totally safe. If I reset the last opcode ID at the top of ConditionalNode::emitCode(), the bug is fixed, which seems to suggest that there is a bug in our register ref counting scheme.
Comment 4 Cameron Zwarich (cpst) 2008-06-30 17:15:38 PDT
The bug is fixed by resetting generator.m_lastOpcodeID between the two calls to generator.emitNode() in CommaNode::emitCode(). At the moment, I am not sure why.
Comment 5 Cameron Zwarich (cpst) 2008-06-30 17:37:07 PDT
There is never a CommaNode with a ConditionalNode on the right, so the ConditionalNode must be emitted by the first emitNode call for some other node type. I'll dump backtraces of ConditionalNode::emitCode() to try and find it. I need to narrow this down, because so much is put in a CommaNode in the obfuscated Google JS.
Comment 6 Cameron Zwarich (cpst) 2008-06-30 17:54:30 PDT
I found the problem. I was merging less and jfalse even when the destination of less was a local. I have a fix, but I need to make a layout test.
Comment 7 Cameron Zwarich (cpst) 2008-06-30 18:02:49 PDT
Created attachment 22012 [details]
Proposed patch

I will write some layout tests, but I'll put this up for review first.
Comment 8 Oliver Hunt 2008-06-30 18:04:28 PDT
Comment on attachment 22012 [details]
Proposed patch

r=me with tests
Comment 9 Cameron Zwarich (cpst) 2008-06-30 18:57:18 PDT
Landed in r34903.