Link to the list of the builds:
===============================
https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%3D%27base%3A%3AJoinString%27%20AND%20product.name%3D%27Chrome%27&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#productversion:1000

Note:
=====
1. Tagging this with Dev blocker for next PGO dev release as we have seen consistently seen 3 digits of crashes on PGO canary and 4 digits of crashes on the latest dev(65.0.3322.4). 
2. Crashes are seen spiked on PGO builds only and seen from chrome version: 65.0.3318.2, hence considering below as the changelog:

https://chromium.googlesource.com/chromium/src/+log/65.0.3317.2..65.0.3318.2?pretty=fuller&n=10000

Not 100% sure as all the associated bugs are Mac related, but tapted@ could these crashes be related to https://chromium-review.googlesource.com/c/chromium/src/+/856016.
If related would request to revert this CL and merge the revert to M-65 branch to unblock tomorrow's Dev release.

Looping Stability sheriff in case the above CL is unrelated and for help in getting the suspected CL reverted from M-65 to unblock tomorrow's Dev release. 


 

 

This is blocking tomorrow's M65 dev release for 15% PGO experiment. Will it be safe to revert culprit cl and merge to M65 branch 3325 ASAP?

https://chromium-review.googlesource.com/c/chromium/src/+/856016 only changes behaviour on Mac. These base::JoinString crashes only happen on not-Mac (95% Windows), so I don't think it's related. Note the only message_center file modified in that CL is cocoa/notification_controller.mm which is not used anywhere except on Mac.

26860 reports have message_center::InnerBoundedLabel::GetSizeForWidthAndLines(int,int) in the stack

24211 are on a specific build -- 65.0.3322.4 - nothing stands out there. Not many CLs (I guess it was a Sunday release or something) - https://chromium.googlesource.com/chromium/src/+log/65.0.3321.0..65.0.3322.0?pretty=fuller&n=10000

They first started appearing in 65.0.3318.2

https://chromium.googlesource.com/chromium/src/+log/65.0.3317.0..65.0.3318.0?pretty=fuller&n=10000 - I don't see anything here either


You're saying this only happens on the Windows PGO build? We may be looking at a compiler codegen/optimization bug. Bumping to someone who may know something about that. I'm out of my depth here.

We (abdulsyed@, pbommana@ and I) decided not to block tomorrow's M65 Dev (15% pgo) release for this. Please get it fixed/merged for next week dev. Thank you.

There's a recent, huge spike of crashes. They have almost the same signature. I found two of the most different ones. The only version to have this issue is: 65.0.3322.4. Agreed that 65.0.3318.2 is first bad build, but crash occurs spottily, so could go back further. Given that this is a windows only bug, seems most likely related to finnur's recent work with native notifications.

https://chromium-review.googlesource.com/840002

Users experienced this crash on the following builds:

Android Beta 64.0.3282.116 -  4.95 CPM, 21 reports, 2 clients (signature base::JoinString)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

I don't think this is related to my changes, which have (up to this point) been purely about native notifications (think Windows Action Center). This, however, looks like a crash related to non-native notifications. 

I can maybe take a quick look at the crash dump tomorrow, but I'm pessimistic about my chances since I don't know this code well. If this is holding up a release (as indicated by private message) then this should probably be looked at by someone else.

Given that we're having trouble narrowing down a culprit CL, adding OWNERS of chrome/browser/notifications to see if they have any idea what's going on.

I looked at this too. I can't find anything obvious in the regression range. It does seeem to be 32-bit, there are hints it's related to some notifications and/or view code, but I can't find anything obvious.

It gave me a good excuse to dig out huangs' old crash graph tool - and I generated this for this magic signature on 65.0.3318.2 'base::JoinString'.

results are here: http://shortn/_9gSzDuKv6Y

Analysis: 

100% of crashes go through message_center::InnerBoundedLabel::GetSizeForWidthAndLines

85% (221/257) go through message_center::UiController::OnBlockingStateChanged

The rest (8) go through message_center::ToastContentsView::OnBoundsAnimationEndedOrCancelled or message_center::MessagePopupCollection::OnNotificationAdded (3) but the (3) end up rejoining the main group (80% - 207/257) in message_center::MessageCenterImpl::AddNotificationImmediately

TL;DR; there's a lot of message_center in there, but that doesn't come out in the regression range (unless I've missed it).

PlatformNotificationServiceImpl::DisplayPersistentNotification means it's a web notification, so some website is providing the text that's being elided then joined. So it appears to be a bug in parsing untrusted input, which seems like a security threat (not sure what severity). There may not be a regression here, just a popular or malicious website that started using a string that triggers the issue. Are sites simply giving us a very long string that we are failing to truncate?

I guess on second thought, if it were a long-time bug surfaced by some site we'd be seeing it on earlier builds.

I think this might be caused by the following CL:
https://chromium-review.googlesource.com/c/chromium/src/+/856016

It changes gfx::ElideRectangleText() from using Typesetter::DEFAULT to using Typesetter::HARFBUZZ.

A failed memmove() in std::string::append() suggests that we're pointing to invalid memory somewhere, but both arguments to base::JoinString() exist, unless the string16 vector in InnerBoundedLabel::GetSizeForWidthAndLines() somehow contains a base::string16 instance pointing to invalid memory?

I won't be able to take another look until Monday. Short-term fix we can try is change gfx::ElideRectangleText() back to Typesetter::DEFAULT.

what's the easiest way of getting this code to trigger on a normal build? Is there a test site for notifications? I will try a win 32-bit asan build on win7 and see if I can repro.

There are lots of online demo sites, such as https://davidwalsh.name/demo/notifications-api.php

> Short-term fix we can try is change gfx::ElideRectangleText() back to Typesetter::DEFAULT.

Per #c3, that CL only has any effect on Mac. See code in https://chromium-review.googlesource.com/c/chromium/src/+/856016/10/ui/gfx/render_text.cc#346

Anywhere apart from Mac, Harfbuzz is the only typesetter. That doesn't rule out something obscure in that CL that I've missed, but I think it is a red herring.

I spent a lot of time looking in #c3, and I think this needs a compiler/PGO expert.

If we're clutching at straws, there's a small refactoring part of that CL that I guess could confuse a code optimizer - I made https://chromium-review.googlesource.com/c/chromium/src/+/890683 to test this theory. It shouldn't have any effect, but I'm out of ideas.

+  sebmarchand@ (PGO expert). PTAL comment #18.

This has consistently been #1 browser crash on Windows PGO builds and has shown 3 digits of crashes on every canary and 4/5 digits of crashes on the last 2 Windows PGO dev releases. 

I think this is happening enough we should consider speculative reverts, at least to narrow down the CL that introduced the regression.

I looked at crash ID a4fbc07f09ce32ba and poked around at the assembly language. The crash happens in memmove in a "rep movsb" instruction. At the time of the crash the ecx/esi/edi (count, source, dest) registers are:

ECX = 0049B484 ESI = 00251000 EDI = 129524CE

Since ESI has just crossed a page boundary it is obviously the problem. Looking up the stack at the "wrapped" variable it has two elements and both of these have strings with addresses in the 0x0d7a65.. range - they are nowhere near the source range. Looking at the JoinStringT function in the call stack the 'sep' variable has an address of 0x0024ebdc and a length of 0x0024ec30. That address would naturally lead to memmove reading from 00251000, especially with a length that large (actual length should be 1).

So the problem is a corrupt separator string piece. This diagnose is supported by noting that the return address in JoinStringT is on the call to sep.AppendToString(&result).

The next step is to find out why 'sep' is bogus. It's a reference to a temporary so this is trickier but I'll take a look. It is possible that this is a code-gen bug, but ???

We won't be blocking tomorrow's M65 Dev release (15% PGO experiment) for this as it has been present since last 2 builds.  Pls let the investigation continue and provide fix before next week Dev. Thank you.

I moved up the stack one level to JoinString and got the address of separator - it is 0x0024EBD8. Looking at that address in memory I see:

0x0024EBD8  0024ebdc 0024ec54 00000001 0024ec08 0024ec10

The '1' looks interesting because that could be the correct length of separator. And indeed if we display this in the watch window:

(base::StringPiece16*)(0x0024ebdc)

then we get a stringpiece that points at L"\n" and has a length of 1.

(base::StringPiece16*)(0x0024ebd8)
+		ptr_	0x0024ebdc L"$\x1"	const wchar_t *
		length_	0x0024ec54	unsigned int
(base::StringPiece16*)(0x0024ebdc)
+		ptr_	0x0024ec54 L"\n"	const wchar_t *
		length_	0x00000001	unsigned int

So, this is looking more and more like a code-gen bug. Somehow the calling code in GetSizeForWidthAndLines is setting up the stack frame incorrectly. We should conditionally disable optimizations for VC++ builds for this function. If I'm feeling energetic I could report a VC++ bug but if it is PGO only then that may be challenging and the benefit is modest given our clang-cl trajectory.

I'll take this bug.

Update: the bad code-gen is in JoinString.

Here is what the stack looks like inside of JoinString:

0024ebd4  116037a8 chrome!message_center::InnerBoundedLabel::GetSizeForWidthAndLines+0x131 
0024ebd8  0024ebdc
0024ebdc  0024ec54
0024ebe0  00000001

The JoinString function looks like this:

string16 JoinString(const std::vector<string16>& parts,
                    StringPiece16 separator) {
  return JoinStringT(parts, separator);
}

So, assuming a default calling convention (not always a safe assumption with PGO) we should expect the stack to be:

    Return-address
    Address of "parts"
    First DWORD of separator (ptr_)
    Second DWORD of separator (length_)

So, the data on the stack is correct - this is most easily seen by noting that 00000001 is where we expect to find separator.length_.

In the JoinString prologue this boilerplate code is executed:

    push        ebp  
    mov         ebp,esp 
    sub         esp,2Ch 

So we end up with the old copy of EBP pushed to 0024ebd0 and EBP then updated to point to that location. So far so normal. This means that separator can be addressed as EBP+0xC.

Here's the rest of the function start:

string16 JoinString(const std::vector<string16>& parts,
                    StringPiece16 separator) {
10211FA0 55                   push        ebp  
10211FA1 8B EC                mov         ebp,esp  
10211FA3 83 EC 2C             sub         esp,2Ch  
10211FA6 A1 BC D0 C3 11       mov         eax,dword ptr [__security_cookie (11C3D0BCh)]  
10211FAB 33 C5                xor         eax,ebp  
10211FAD 89 45 FC             mov         dword ptr [ebp-4],eax  
  return JoinStringT(parts, separator);
10211FB0 8B 45 08             mov         eax,dword ptr [separator] (+08h)  - loading separator.ptr_ - from wrong location (should be +0Ch)
10211FB3 53                   push        ebx  
10211FB4 8B 5D 0C             mov         ebx,dword ptr [ebp+0Ch]  - loading separator.length_ - from wrong location (should be +10h)
10211FB7 56                   push        esi  
10211FB8 8B F2                mov         esi,edx  
10211FBA 89 45 DC             mov         dword ptr [ebp-24h],eax  - storing separator.ptr_ as local


Note that separator.ptr_ and separator.length_ are loaded from the wrong locations. Later on ebx and ebp-24h are used:

1021205A 8B 7D DC             mov         edi,dword ptr [ebp-24h]  - loading separator.ptr_ from separator copy
1021205D 0F 1F 00             nop         dword ptr [eax]  
10212060 85 DB                test        ebx,ebx  - testing separator.length_
10212062 74 0A                je          base::JoinString+0CEh (1021206Eh)  
10212064 53                   push        ebx  - pushing separator.length_
10212065 57                   push        edi  - pushing separator.ptr_
10212066 8D 4D E4             lea         ecx,[ebp-1Ch]  - loading the address of t
 - This is the call that crashes.
10212069 E8 3D 1C 93 FF       call        std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> >::append (0FB43CABh)  

All of this is consistent with the crash. The fix remains mostly the same - conditional disabling of optimizations - but it is JoinString that needs this. Doing so will *probably* fix the bug, but PGO optimization bugs are not always well contained, so we will see.

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

commit 4f4f08c928de6f68817b2d1ee3926b359e44cd6c
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Mon Jan 29 23:35:46 2018

Work around VC++ code-ben  bug 804884 

VC++ PGO builds crash deep inside memmove, as called by
message_center::InnerBoundedLabel::GetSizeForWidthAndLines which calls
base::JoinString and then ultimately basic_string<>::append and memmove.
Inspection of the stack and the generated code shows that
base::JoinString is called correctly but that base::JoinString (or the
inlined JoinStringT and three other levels of inlining) fail to read
separator off the stack correctly - they read it from an address that is
four bytes too early. This leads to copying too many bytes from the
wrong address.

Fully disabling optimizations in VC++ builds for the affected function
seems like the right fix. If time permits then a reduction and VC++ bug
will be created. However if this is a PGO-only bug then...

Bug:  804884 
Change-Id: I80fd8e693a35b26a386cd99a1b17c54bede95d5c
Reviewed-on: https://chromium-review.googlesource.com/891569
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532658}
[modify] https://crrev.com/4f4f08c928de6f68817b2d1ee3926b359e44cd6c/base/strings/string_util.cc

Just to update, latest PGO canary version: 66.0.3335.2 has not reported any crash instances so far, for the magic signature 'base::JoinString'. Canary versions prior to today have consistently been reported 3 digits of crashes on PGO builds.

Based on the available crash data this looks to be good.

Link to crashes for the latest PGO canary version 66.0.3335.2:

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20product.version%3D%2766.0.3335.2%27%20AND%20expanded_custom_data.ChromeCrashProto.channel%3D%27canary%27&sql_dialect=googlesql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#+magicsignature

Thanks for verifying the fix. I'm going to close this as fixed as I don't think any more actions are required.

[Auto-generated comment by a script] We noticed that this issue is targeted for M-65; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-65 label, otherwise remove Merge-TBD label. Thanks.

Do we need a merge to M65? May be not as this bug is for pgo and M65 Beta/stable will be clang by default unless something goes wrong with M64 stable (clang).

Reopening because a merge is still needed. Merge request added.

My workaround only affects VC++ builds and therefore has no effect on beta or stable builds. This makes it a particularly safe merge that is well worth doing just to avoid distractions from crashes in the dev branch. So, yeah, might as well merge.

Thank you auto-generated comment.

Approving merge to M65 branch 3325 based on comment #31. 

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

commit 4e185a58c29c0565e774abf16c234c81069c8a2d
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Wed Jan 31 21:39:16 2018

Work around VC++ code-ben  bug 804884 

VC++ PGO builds crash deep inside memmove, as called by
message_center::InnerBoundedLabel::GetSizeForWidthAndLines which calls
base::JoinString and then ultimately basic_string<>::append and memmove.
Inspection of the stack and the generated code shows that
base::JoinString is called correctly but that base::JoinString (or the
inlined JoinStringT and three other levels of inlining) fail to read
separator off the stack correctly - they read it from an address that is
four bytes too early. This leads to copying too many bytes from the
wrong address.

Fully disabling optimizations in VC++ builds for the affected function
seems like the right fix. If time permits then a reduction and VC++ bug
will be created. However if this is a PGO-only bug then...

Bug:  804884 
Change-Id: I80fd8e693a35b26a386cd99a1b17c54bede95d5c
Reviewed-on: https://chromium-review.googlesource.com/891569
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#532658}(cherry picked from commit 4f4f08c928de6f68817b2d1ee3926b359e44cd6c)
Reviewed-on: https://chromium-review.googlesource.com/896183
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#214}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/4e185a58c29c0565e774abf16c234c81069c8a2d/base/strings/string_util.cc

Okay, *now* closing as fixed.

Removing "Merge-TBD" label as it is already merged to M65 at #33.

Issue 812223 has been merged into this issue.

Issue 813208 has been merged into this issue.

Issue 813314 has been merged into this issue.