Odd discrepancy between CQ and main waterfall

[Colin Blundell]

Hi Pawel,

I know that you're interested in discrepancies between the CQ and the waterfall, so I thought you might want to know about this issue.

https://codereview.chromium.org/1313213005/ passed the CQ (and earlier trybot runs) with no problems. However, it had a bug: changes I made to browsertests resulted in them dereferencing a scoped_ptr that contained null. scoped_ptr.h has an assertion that fails in this case, which is indeed what got triggered on the main waterfall: http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/11102/steps/browser_tests%20on%20Mac-10.9/logs/PolicyVariationsServiceTest.VariationsURLIsValid, for example.

It's not obvious to me why the CQ/trybot runs succeeded in this case.

Thanks,

Colin

[Paweł Hajdan, Jr.]

This is a great catch. Could you file a bug for it?

My current theory: we don't have Debug coverage on CQ, but we enable DCHECKs in Release mode. However, in this case it's an assert (Assertion failed: (impl_.get() != nullptr), function operator->, file ../../base/memory/scoped_ptr.h, line 386) so it's not affected by this.

Do you know why it's an assert (+chromium-dev)? Are there some additional #defines we could turn on the CQ trybots to enable asserts there if this can't be changed?

Paweł

[Colin Blundell]

On Wed, Sep 2, 2015 at 4:07 PM Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

This is a great catch. Could you file a bug for it?

crbug.com/527444

 

My current theory: we don't have Debug coverage on CQ, but we enable DCHECKs in Release mode. However, in this case it's an assert (Assertion failed: (impl_.get() != nullptr), function operator->, file ../../base/memory/scoped_ptr.h, line 386) so it's not affected by this.

Do you know why it's an assert (+chromium-dev)?

Nope!

[Scott Graham]

On Wed, Sep 2, 2015 at 8:30 AM, Colin Blundell <blun...@chromium.org> wrote:

On Wed, Sep 2, 2015 at 4:07 PM Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

This is a great catch. Could you file a bug for it?

crbug.com/527444

 

My current theory: we don't have Debug coverage on CQ, but we enable DCHECKs in Release mode. However, in this case it's an assert (Assertion failed: (impl_.get() != nullptr), function operator->, file ../../base/memory/scoped_ptr.h, line 386) so it's not affected by this.

Do you know why it's an assert (+chromium-dev)?

Nope!

I was wondering that the other day when I happened to step into that code.

It looks like it might be to match the implementation that's internal to Google? +dcheng as original author https://chromium.googlesource.com/chromium/src/+/9961abd411eab6ec1cf9421078dfdaa001a7e2b5%5E%21/#F0

It doesn't seem particularly valuable to maintain that, if that's the reason.

 

Are there some additional #defines we could turn on the CQ trybots to enable asserts there if this can't be changed?

Paweł

On Wed, Sep 2, 2015 at 3:37 PM, Colin Blundell <blun...@chromium.org> wrote:

Hi Pawel,

I know that you're interested in discrepancies between the CQ and the waterfall, so I thought you might want to know about this issue.

https://codereview.chromium.org/1313213005/ passed the CQ (and earlier trybot runs) with no problems. However, it had a bug: changes I made to browsertests resulted in them dereferencing a scoped_ptr that contained null. scoped_ptr.h has an assertion that fails in this case, which is indeed what got triggered on the main waterfall: http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/11102/steps/browser_tests%20on%20Mac-10.9/logs/PolicyVariationsServiceTest.VariationsURLIsValid, for example.

It's not obvious to me why the CQ/trybot runs succeeded in this case.

Thanks,

Colin

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Scott Hess]

On Wed, Sep 2, 2015 at 10:18 AM, Scott Graham <sco...@chromium.org> wrote:

On Wed, Sep 2, 2015 at 8:30 AM, Colin Blundell <blun...@chromium.org> wrote:

On Wed, Sep 2, 2015 at 4:07 PM Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

This is a great catch. Could you file a bug for it?

crbug.com/527444

 

My current theory: we don't have Debug coverage on CQ, but we enable DCHECKs in Release mode. However, in this case it's an assert (Assertion failed: (impl_.get() != nullptr), function operator->, file ../../base/memory/scoped_ptr.h, line 386) so it's not affected by this.

Do you know why it's an assert (+chromium-dev)?

Nope!

I was wondering that the other day when I happened to step into that code.

It looks like it might be to match the implementation that's internal to Google? +dcheng as original author https://chromium.googlesource.com/chromium/src/+/9961abd411eab6ec1cf9421078dfdaa001a7e2b5%5E%21/#F0

It doesn't seem particularly valuable to maintain that, if that's the reason.

I always assumed it was because it was a header file and logging.h was deemed too heavyweight.

-scott

 

[Dana Jansens]

On Wed, Sep 2, 2015 at 7:06 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

This is a great catch. Could you file a bug for it?

My current theory: we don't have Debug coverage on CQ, but we enable DCHECKs in Release mode. However, in this case it's an assert (Assertion failed: (impl_.get() != nullptr), function operator->, file ../../base/memory/scoped_ptr.h, line 386) so it's not affected by this.

Do you know why it's an assert (+chromium-dev)? Are there some additional #defines we could turn on the CQ trybots to enable asserts there if this can't be changed?

Paweł

On Wed, Sep 2, 2015 at 3:37 PM, Colin Blundell <blun...@chromium.org> wrote:

Hi Pawel,

I know that you're interested in discrepancies between the CQ and the waterfall, so I thought you might want to know about this issue.

https://codereview.chromium.org/1313213005/ passed the CQ (and earlier trybot runs) with no problems. However, it had a bug: changes I made to browsertests resulted in them dereferencing a scoped_ptr that contained null. scoped_ptr.h has an assertion that fails

How do you deref null and not segfault?

 

in this case, which is indeed what got triggered on the main waterfall: http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/11102/steps/browser_tests%20on%20Mac-10.9/logs/PolicyVariationsServiceTest.VariationsURLIsValid, for example.

It's not obvious to me why the CQ/trybot runs succeeded in this case.

Thanks,

Colin

[Antoine Labour]

On Wed, Sep 2, 2015 at 10:40 AM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, Sep 2, 2015 at 7:06 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

This is a great catch. Could you file a bug for it?

My current theory: we don't have Debug coverage on CQ, but we enable DCHECKs in Release mode. However, in this case it's an assert (Assertion failed: (impl_.get() != nullptr), function operator->, file ../../base/memory/scoped_ptr.h, line 386) so it's not affected by this.

Do you know why it's an assert (+chromium-dev)? Are there some additional #defines we could turn on the CQ trybots to enable asserts there if this can't be changed?

Paweł

On Wed, Sep 2, 2015 at 3:37 PM, Colin Blundell <blun...@chromium.org> wrote:

Hi Pawel,

I know that you're interested in discrepancies between the CQ and the waterfall, so I thought you might want to know about this issue.

https://codereview.chromium.org/1313213005/ passed the CQ (and earlier trybot runs) with no problems. However, it had a bug: changes I made to browsertests resulted in them dereferencing a scoped_ptr that contained null. scoped_ptr.h has an assertion that fails

How do you deref null and not segfault?

operator-> returns the pointer, and in some cases you can syntactically deref a pointer without causing a segfault (e.g. calling a non-virtual method that doesn't use |this| or a field).

Back to the original question, even if we leave it as an assert, should the Release+DCHECK build be built without NDEBUG?

Antoine

 

in this case, which is indeed what got triggered on the main waterfall: http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/11102/steps/browser_tests%20on%20Mac-10.9/logs/PolicyVariationsServiceTest.VariationsURLIsValid, for example.

It's not obvious to me why the CQ/trybot runs succeeded in this case.

Thanks,

Colin

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Alexei Svitkine]

If you call an instance method on an object that doesn't actually touch any of its members, |this| won't be deref'd so it may happily execute without crashing even if the object is null.

[Jeffrey Yasskin]

I don't have the reference in front of me, so I could be wrong, but I believe a call through a null 'this' is always undefined behavior, even if the compiler happens to make it behave the way the author expected.

&*null is the one that depends on how you squint at the standard.

[Paweł Hajdan, Jr.]

Do you see any reasons not to check the assert into DCHECK?

I'd be hesitating to disable NDEBUG on trybots, because it'd probably change too much (like enable possibly slower code, disable coverage for Release codepaths where they differ from Debug)...

Paweł

[Colin Blundell]

On Thu, Sep 3, 2015 at 3:37 PM Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Do you see any reasons not to check the assert into DCHECK?

I think that +Daniel Cheng (who somehow got dropped back off) should weigh in here.

 

I'd be hesitating to disable NDEBUG on trybots, because it'd probably change too much (like enable possibly slower code, disable coverage for Release codepaths where they differ from Debug)...

A naive question: Am I understanding correctly that there is *no* debug test coverage in the CQ? My CL failed across several bots on the main waterfall, for example; having CQ coverage for any one of those configurations would have blocked the CL from landing.

[Bence Béky]

On Thu, Sep 3, 2015 at 9:40 AM, Colin Blundell <blun...@chromium.org> wrote:
>
> A naive question: Am I understanding correctly that there is *no* debug test coverage in the CQ? My CL failed across several bots on the main waterfall, for example; having CQ coverage for any one of those configurations would have blocked the CL from landing.
>

I would like to second that.

Also, on the flip side, it seems like there is no coverage in the CQ
without DCHECKs. I once foolishly relied on the side effect of an
expression that I put in a DCHECK, which passed the CQ but then broke
on "regular" production builds (that is, ones without DCHECKs).
Having CQ coverage of code compiled without DCHECKs would have
prevented that CL from landing.

Thanks,

Bence

[John Abd-El-Malek]

This is a known side-effect of the tradeoffs we make. In practice, it comes up very infrequently.

Running tests in debug is much slower, and we've found that running them with release-with-asserts gets most of the benefit.

 

Thanks,

Bence

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CACMu3tqFQE7okzKWjpK1nmpKnAzR0Zq_HJPu82dj4%3DUk%3DLgdkw%40mail.gmail.com.

[John Abd-El-Malek]

On Thu, Sep 3, 2015 at 6:37 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Do you see any reasons not to check the assert into DCHECK?

I'd be hesitating to disable NDEBUG on trybots, because it'd probably change too much (like enable possibly slower code, disable coverage for Release codepaths where they differ from Debug)...

+1, let's just turn the assert into a DCHECK.

 

Paweł

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPaRcyj1yi8N4gptKHZShS%2BxbQ7ziWN%2Bi8NnbNcxkFD5uw%40mail.gmail.com.

[Colin Blundell]

On Thu, Sep 3, 2015 at 4:22 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Sep 3, 2015 at 6:37 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Do you see any reasons not to check the assert into DCHECK?

I'd be hesitating to disable NDEBUG on trybots, because it'd probably change too much (like enable possibly slower code, disable coverage for Release codepaths where they differ from Debug)...

+1, let's just turn the assert into a DCHECK.

 

I'll do this.

[Dana Jansens]

On Thu, Sep 3, 2015 at 8:55 AM, Colin Blundell <blun...@chromium.org> wrote:

On Thu, Sep 3, 2015 at 4:22 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Sep 3, 2015 at 6:37 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Do you see any reasons not to check the assert into DCHECK?

I'd be hesitating to disable NDEBUG on trybots, because it'd probably change too much (like enable possibly slower code, disable coverage for Release codepaths where they differ from Debug)...

+1, let's just turn the assert into a DCHECK.

 

I'll do this.

The LOG macro in base/logging.h conflicts with the LOG macro in WTF. Since scoped_ptr.h is included everywhere, it is included into things that also include WTF.

I once tried to rename the blink LOG macro: https://codereview.chromium.org/13643002/. There's a bunch of conversation by email that didn't correctly end up on that CL here: https://groups.google.com/a/chromium.org/d/msg/blink-reviews/9uua982NIYI/Ey0NL8iPYYEJ

[Jeremy Roman]

It was renamed to WTF_LOG: https://chromium.googlesource.com/chromium/blink/+/982944a9bb0796cee538ee092cfa1f0463dd9139

[Dana Jansens]

On Thu, Sep 3, 2015 at 10:41 AM, Jeremy Roman <jbr...@chromium.org> wrote:

It was renamed to WTF_LOG: https://chromium.googlesource.com/chromium/blink/+/982944a9bb0796cee538ee092cfa1f0463dd9139

Oh, hooray :) That makes it a lot more feasible.

[Colin Blundell]

It turns out that there's code in Blink that uses scoped_ptr (#til) and gets linked into targets that get built within Blink (e.g., WebGL2RenderingContextBase.cpp, which gets linked into Blink's modules target). Adding a reference to logging.h within scoped_ptr.h causes linkage to fail for these targets due to LogMessage() not being defined (see http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/117103/steps/compile%20%28with%20patch%29/logs/stdio for an example). Based on what I know of the relationship between Blink and Chromium I assume that adding a dependency from Blink to //base is a non-starter, which kills this change.

[Ojan Vafai]

BCC: infra-dev, chromium-dev

+blink-dev 

There was strong disagreement about replacing WTF with base or vice versa, but I don't know if we've made a clear decision with respect to things in base that have no WTF equivalent and/or depending on base in general.

Anyone recall where we left this off?

[Jeremy Roman]

Sent https://codereview.chromium.org/1310743006 for review; I don't think Blink should be using scoped_ptr there.

[Colin Blundell]

Thanks, Jeremy! Interestingly, it turns out that removing usage of scoped_ptr within //third_party/WebKit itself is not enough.

//gin/public includes scoped_ptr.h in header files that are included from within //third_party/WebKit. //base/logging.h redefines assert() to use LogMessage(). Thus, any code in //third_party/WebKit that (a) directly or transitively includes the files from //gin/public, (b) uses assert(), and (c) gets linked into any target built in Blink that doesn't depend on //gin will cause that target to fail at the link step. Again, an example here is Blink's modules target; an example offending file is AXNodeObject.cpp.  

At this point, I'm out of my depth as to whether/how to proceed. Naively, it seems like a bit of a footgun to me that (a) //gin/public includes files from //base and (b) there are targets within //third_party/WebKit that don't depend on //gin but do link files that include //gin/public headers that reference //base. Maybe this is WAI, and it's common knowledge that the //base files in question should not introduce any link-time dependencies? In that case, I'll just abandon this patch. Otherwise, we could remove the usage of scoped_ptr.h within //gin/public (and then see what the next problem is ;).

~Colin

[Jochen Eisinger]

//gin/public was designed to be safely includable from webkit, so it should not include a scoped_ptr.

[Colin Blundell]

Thanks, Jochen. So should those scoped_ptrs just be raw pointers with comments about ownership?

[Jochen Eisinger]

What about creating an internal class e.g. IsolateHolder::OwnedData - that way we'd keep the automatic memory handing.

We should probably also add DEPS rules to enforce this

[Jeremy Roman]

On Mon, Sep 7, 2015 at 11:08 AM, Colin Blundell <blun...@chromium.org> wrote:

Thanks, Jeremy! Interestingly, it turns out that removing usage of scoped_ptr within //third_party/WebKit itself is not enough.

//gin/public includes scoped_ptr.h in header files that are included from within //third_party/WebKit. //base/logging.h redefines assert() to use LogMessage(). Thus, any code in //third_party/WebKit that (a) directly or transitively includes the files from //gin/public, (b) uses assert(), and (c) gets linked into any target built in Blink that doesn't depend on //gin will cause that target to fail at the link step. Again, an example here is Blink's modules target; an example offending file is AXNodeObject.cpp.  

Blink code should be using ASSERT from WTF. Mailed https://codereview.chromium.org/1305353011.

[Colin Blundell]

Thanks, Jeremy!

This fix got us past the WebKit failures. There are now a couple similar failures: in chrome_elf on Windows and remoting on Mac. In both cases it's not clear to me whether a dependency on //base is actually forbidden or not. Before investigating that question, I first just added the dependencies on //base in the failing targets to verify that the problems were fixed by doing so, and surprisingly found that the additions of those dependencies didn't cause the link failures to go away. I'm out of spare cycles to keep tracking this down at the moment, but if anyone wants to do further investigation, be my guest!

https://codereview.chromium.org/1305973011/ (PS2 is after Jeremy's fix; PS3 contains my quick-and-dirty dependency additions to try to fix the Mac and Windows link failures. Note that the ChromeOS failures are an unrelated problem that was happening at the time).

~Colin

[Paweł Hajdan, Jr.]

Just checking back, what's the status here?

Colin filed https://code.google.com/p/chromium/issues/detail?id=527444 for the original issue.

https://codereview.chromium.org/1305973011/ didn't seem to land, with the reason related to WebKit dependencies on base. This might have changed since then.

The asserts in scoped_ptr.h seem to still be there. :-/

Paweł

[Colin Blundell]

On Tue, Jan 12, 2016 at 3:49 PM Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Just checking back, what's the status here?

Colin filed https://code.google.com/p/chromium/issues/detail?id=527444 for the original issue.

https://codereview.chromium.org/1305973011/ didn't seem to land, with the reason related to WebKit dependencies on base. This might have changed since then.

The asserts in scoped_ptr.h seem to still be there. :-/

Yep, the asserts are still there. That CL didn't land for the reasons I described below (note that it's not about WebKit dependencies on //base, as jroman@ helpfully removed those). The status hasn't changed, as I don't have more spare cycles to put into investigation. If someone else wants to take up the baton, feel free!

[Nico Weber]

gin could now use unique_ptr instead of scoped_ptr (didn't read the whole thread, not sure if that'd help with anything).

[Colin Blundell]

Signal-boosting the actual outstanding problems. Maybe these could also now be solved using unique_ptr...

There are now a couple similar failures: in chrome_elf on Windows and remoting on Mac. In both cases it's not clear to me whether a dependency on //base is actually forbidden or not. Before investigating that question, I first just added the dependencies on //base in the failing targets to verify that the problems were fixed by doing so, and surprisingly found that the additions of those dependencies didn't cause the link failures to go away. I'm out of spare cycles to keep tracking this down at the moment, but if anyone wants to do further investigation, be my guest!

https://codereview.chromium.org/1305973011/ (PS2 is after Jeremy's fix; PS3 contains my quick-and-dirty dependency additions to try to fix the Mac and Windows link failures. Note that the ChromeOS failures are an unrelated problem that was happening at the time).