New issue
Advanced search Search tips

Issue 593446 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

Assertion `overrides_blink != overrides_non_blink' failed

Project Member Reported by danakj@chromium.org, Mar 9 2016

Issue description

This fires for third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp

and most files it seems when running the tool on bindings/core/v8.
 
This means we have a method overriding a class from blink and a class from not-blink without the same parent, so the tool renaming the blink method would cause a behaviour change.
Ah it was destructors, which we exclude, but after we checked if they are a blink method. Reordering solves it.
Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e27ff645c0bfb460855b825f08beda583d71a8e5

commit e27ff645c0bfb460855b825f08beda583d71a8e5
Author: danakj <danakj@chromium.org>
Date: Wed Mar 09 21:29:59 2016

rewrite_to_chrome_style: Exclude methods before checking IsBlinkOrWTF

The IsBlinkOrWTFMethod asserts that the method is not doing crazy
overrides, but it will fire for destructors which we aren't trying to
rename anyways. So exclude those before trying to see if we want to
rename it.

R=dcheng
BUG= 593446 

Review URL: https://codereview.chromium.org/1778963003

Cr-Commit-Position: refs/heads/master@{#380228}

[modify] https://crrev.com/e27ff645c0bfb460855b825f08beda583d71a8e5/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Status: Started (was: Fixed)
Still happening

rewrite_to_chrome_style: /usr/local/google/home/danakj/s/c/src/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:121: bool (anonymous namespace)::IsBlinkOrWTFMethod(const clang::CXXMethodDecl &): Assertion `overrides_blink != overrides_non_blink' failed.

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_view_impl.h&l=447

RenderViewImpl::convertViewportToWindow overrides:
- A virtual method in WebWidgetClient (from blink)
- A purevirtual method in RenderView (not from blink)
- A overridden method in RenderWidget (not from blink)
  - A virtual method in WebWidgetClient (again?)
  - An overridden method in RenderViewImpl (not from blink)
  ...

I feel like codesearch makes an inheritance cycle at some point.
OK It's

RenderWidget inherits WebWidgetClient
- provides convertViewportToWindow

RenderViewImpl inherits RenderView
- wants to expose RenderWidget's convertViewportToWindow
- does so by making a virtual on RenderView with the *SAME NAME*
- the override in RenderViewImpl calls RenderWidget's

T_T

Going to rename the one on RenderView.
Another one.

InspectorInspectorAgent inherits a disable() method from:
- InspectorAgent (which is in blink::)
  https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/inspector/InspectorBaseAgent.h&rcl=1457541024&l=63
- Dispatcher::InspectorCommandHandler (which is in blink::protocol:: and is in gen/ so we don't rename it)
  https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blink/platform/inspector_protocol/Dispatcher.h&rcl=1457541024&l=42
  
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dea2efb645e7ac6bae644658f963b096c2800652

commit dea2efb645e7ac6bae644658f963b096c2800652
Author: danakj <danakj@chromium.org>
Date: Thu Mar 10 19:13:17 2016

content: Rename virtual method on RenderView to not (ab)use blink style

The RenderView::convertViewportToWindow method uses blink style to
create a horrorshow inheritance hierarchy problem, where the same
named method in RenderViewImpl inherits this method from two places
(one in blink one in content) from RenderView and from RenderWidget.

Fix this by renaming the method on RenderView to not use the same
name as the blink method (even after blink uses chromium style).

R=avi@chromium.org,jochen@chromium.org
BUG= 593446 

Review URL: https://codereview.chromium.org/1781873002

Cr-Commit-Position: refs/heads/master@{#380441}

[modify] https://crrev.com/dea2efb645e7ac6bae644658f963b096c2800652/content/public/renderer/render_view.h
[modify] https://crrev.com/dea2efb645e7ac6bae644658f963b096c2800652/content/renderer/browser_plugin/browser_plugin.cc
[modify] https://crrev.com/dea2efb645e7ac6bae644658f963b096c2800652/content/renderer/render_view_impl.cc
[modify] https://crrev.com/dea2efb645e7ac6bae644658f963b096c2800652/content/renderer/render_view_impl.h
[modify] https://crrev.com/dea2efb645e7ac6bae644658f963b096c2800652/extensions/renderer/extension_helper.cc

And one in Source/core/testing/InternalSettings.h:

InternalSettings defines a virtual trace() method via the DECLARE_VIRTUAL_TRACE() macro.

InternalSettings derives from:

1) InternalSettingsGenerated, which uses DECLARE_VIRTUAL_TRACE() to make a trace() function also. This class lives in gen/ so it will not be renamed (we think, except it would cuz it's a macro..)

2)_HeapSupplement which derives from SupplementableBase<T, true> which derives from SupplementableTracing<T, isGarbageCollected>.
- SupplementableTracing is in platform/ and uses DEFINE_INLINE_VIRTUAL_TRACE() to define a virtual trace() method.
This one can be solved be teaching the tool to ignore trace() a little earlier.
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8821e2f26fadc019c3b82807854d03ae203d5de

commit e8821e2f26fadc019c3b82807854d03ae203d5de
Author: danakj <danakj@chromium.org>
Date: Fri Mar 11 20:43:51 2016

rewrite_to_chrome_style: Consistently decide if things are in blink.

The matcher rules would check IsInBlink and then exclude stuff in gen/.
But the method-matcher is special and it has to look at overrides, and
it would look check IsInBlink but would not exclude gen/.

2ndly, the matcher rules would check that any ancestor namespace was
blink or WTF, but the method-matcher would only check the first
namespace.

This makes them consistent. IsInBlinkOrWTF will always return false for
things in gen/, both for the matcher rules and for the method-matcher.
And both will check any ancestor namespace, not just the next namespace.

R=dcheng
BUG= 593446 

Review URL: https://codereview.chromium.org/1785563002

Cr-Commit-Position: refs/heads/master@{#380720}

[modify] https://crrev.com/e8821e2f26fadc019c3b82807854d03ae203d5de/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
[modify] https://crrev.com/e8821e2f26fadc019c3b82807854d03ae203d5de/tools/clang/rewrite_to_chrome_style/tests/gen/thing.h
[modify] https://crrev.com/e8821e2f26fadc019c3b82807854d03ae203d5de/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc
[modify] https://crrev.com/e8821e2f26fadc019c3b82807854d03ae203d5de/tools/clang/rewrite_to_chrome_style/tests/methods-original.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Mar 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27f1728f9c2cdfb9995c671783edd55bd66470be

commit 27f1728f9c2cdfb9995c671783edd55bd66470be
Author: danakj <danakj@chromium.org>
Date: Fri Mar 11 21:21:07 2016

rewrite_to_chrome_style: Remove blacklisted methods with matcher

Consider blacklisted methods when looking at deciding to match against
them or not rather than deciding to not rename them much later.

R=dcheng
BUG= 593446 

Review URL: https://codereview.chromium.org/1783923003

Cr-Commit-Position: refs/heads/master@{#380735}

[modify] https://crrev.com/27f1728f9c2cdfb9995c671783edd55bd66470be/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec30d6b644ed9cd739b3b65efca762e1b1846cfc

commit ec30d6b644ed9cd739b3b65efca762e1b1846cfc
Author: danakj <danakj@chromium.org>
Date: Fri Mar 11 23:00:07 2016

rewrite_to_chrome_style: Blacklist InspectorAgent::disable

This method overrides stuff in gen/ and stuff from blink:: and if we
rename it without renaming both we'll break things. So don't rename
it at all.

R=dcheng
BUG= 593446 

Review URL: https://codereview.chromium.org/1782293003

Cr-Commit-Position: refs/heads/master@{#380773}

[modify] https://crrev.com/ec30d6b644ed9cd739b3b65efca762e1b1846cfc/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Status: Fixed (was: Started)
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/535b89a267d880c41efed60526213483ae79e707

commit 535b89a267d880c41efed60526213483ae79e707
Author: lukasza <lukasza@chromium.org>
Date: Thu Jan 19 19:33:55 2017

Make sure InspectorBaseAgent::disable is not renamed.

When we started blacklisting renaming of "disable" method (in r380773)
the virtual method was implemented by InspectorAgent class.  Since then
(in 396629) the virtual method was moved into InspectorBaseAgent class
and consequently blacklisting stopped working - this CL fixes this.

I've tested this CL by running the clang tool on
third_party/WebKit/Source/modules/storage/StorageNamespaceController.cpp
and verifying that "disable" method in core/inspector/InspectorBaseAgent.h
was not renamed after this CL (and was renamed before this CL).

BUG= 593446 

Review-Url: https://codereview.chromium.org/2646803002
Cr-Commit-Position: refs/heads/master@{#444815}

[modify] https://crrev.com/535b89a267d880c41efed60526213483ae79e707/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Sign in to add a comment