Bug 19830

Summary: REGRESSION (r34883): Google Reader doesn't show up feed list on sidebar
Product: WebKit Reporter: Ismail Donmez <ismail>
Component: New BugsAssignee: Cameron Zwarich (cpst) <zwarich>
Status: RESOLVED FIXED    
Severity: Normal CC: zwarich
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Screenshot showing the issue.
none
Proposed patch oliver: review+

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.