Bug 91451 - REGRESSION: RenderInline::absoluteQuads produces incorrect results for fixed position.
Summary: REGRESSION: RenderInline::absoluteQuads produces incorrect results for fixed ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kiran Muppala
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-07-16 17:26 PDT by Kiran Muppala
Modified: 2012-07-17 13:06 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.84 KB, patch)
2012-07-16 18:19 PDT, Kiran Muppala
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kiran Muppala 2012-07-16 17:26:54 PDT
RenderInline::absoluteQuads is producing incorrect results if having a fixed position.  This leads to assertion failures in RenderGeometryMap::absoluteRect when opening webpages with such content.

On investigation, found that the RenderGeometryStep copy constructor was not coping the offset and hence the result was incorrect.  However, while looking at RenderGeometryStep's copy constructor I also observed that in general it is not safe to copy a RenderGeometryMap, since it's individual step's have a owned pointer to their respective transforms.  So, the broader issue here is that RenderInline::absoluteQuads creates copies of RenderGeometryMap indirectly by passing copies of AbsoluteQuadsGeneratorContext.  To avoid issues with copying a GeometryMap it is better to modify RenderInline::absoluteQuads to pass AbsoluteQuadsGeneratorContext by reference instead.
Comment 1 Kiran Muppala 2012-07-16 17:27:26 PDT
<rdar://problem/11839315>
Comment 2 Kiran Muppala 2012-07-16 18:19:46 PDT
Created attachment 152672 [details]
Patch
Comment 3 WebKit Review Bot 2012-07-16 20:26:13 PDT
Comment on attachment 152672 [details]
Patch

Clearing flags on attachment: 152672

Committed r122802: <http://trac.webkit.org/changeset/122802>
Comment 4 WebKit Review Bot 2012-07-16 20:26:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Daniel Bates 2012-07-17 00:23:57 PDT
(In reply to comment #3)
> (From update of attachment 152672 [details])
> Clearing flags on attachment: 152672
> 
> Committed r122802: <http://trac.webkit.org/changeset/122802>

For some reason this commit caused the Chromium Mac build to fail because the compiler detected that the private instance variable AbsoluteQuadsGeneratorContext::m_wasFixed was unused (see remark (*)):

/Volumes/data/b/WebKit-BuildSlave/chromium-mac-release/build/Source/WebCore/WebCore.gyp/../rendering/RenderInline.cpp:665:11: error: private field 'm_wasFixed' is not used [-Werror,-Wunused-private-field]
    bool* m_wasFixed;

<http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/40008/steps/compile-webkit/logs/stdio>

Removed unused private instance variable AbsoluteQuadsGeneratorContext::m_wasFixed and committed this in <http://trac.webkit.org/changeset/122812>.

(*) AbsoluteQuadsGeneratorContext::m_wasFixed has remained unused since it was added in <http://trac.webkit.org/changeset/116718> (bug #85725).
Comment 6 Kiran Muppala 2012-07-17 13:06:23 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 152672 [details] [details])
> > Clearing flags on attachment: 152672
> > 
> > Committed r122802: <http://trac.webkit.org/changeset/122802>
> 
> For some reason this commit caused the Chromium Mac build to fail because the compiler detected that the private instance variable AbsoluteQuadsGeneratorContext::m_wasFixed was unused (see remark (*)):
> 
> /Volumes/data/b/WebKit-BuildSlave/chromium-mac-release/build/Source/WebCore/WebCore.gyp/../rendering/RenderInline.cpp:665:11: error: private field 'm_wasFixed' is not used [-Werror,-Wunused-private-field]
>     bool* m_wasFixed;
> 
> <http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/40008/steps/compile-webkit/logs/stdio>
> 
> Removed unused private instance variable AbsoluteQuadsGeneratorContext::m_wasFixed and committed this in <http://trac.webkit.org/changeset/122812>.
> 
> (*) AbsoluteQuadsGeneratorContext::m_wasFixed has remained unused since it was added in <http://trac.webkit.org/changeset/116718> (bug #85725).

I am not sure why commit r122802, started to trigger the failure, may be because prior to it the sole instantiation of AbsoluteQuadsGeneratorContext was a temporary and that possibly allowed the compiler to be lax with the checks.    Thanks for the fix.  The code is cleaner without the unused variable for sure.