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+

Ismail Donmez
Reported 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.
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+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ismail Donmez
Comment 1 2008-06-30 12:41:24 PDT
Created attachment 22010 [details] Screenshot showing the issue.
Cameron Zwarich (cpst)
Comment 2 2008-06-30 13:46:56 PDT
I can confirm that this regresses with r34883.
Cameron Zwarich (cpst)
Comment 3 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.
Cameron Zwarich (cpst)
Comment 4 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.
Cameron Zwarich (cpst)
Comment 5 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.
Cameron Zwarich (cpst)
Comment 6 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.
Cameron Zwarich (cpst)
Comment 7 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.
Oliver Hunt
Comment 8 2008-06-30 18:04:28 PDT
Comment on attachment 22012 [details] Proposed patch r=me with tests
Cameron Zwarich (cpst)
Comment 9 2008-06-30 18:57:18 PDT
Landed in r34903.
Note You need to log in before you can comment on or make changes to this bug.