Removed Internal>Mojo because I saw another crash which wasn't related to mojo.

http/tests/serviceworker/fetch-response-taint.html failed with the following log.

==
[1:1:1215/125758.443811:1821588629999:FATAL:StringView.cpp(19)] Check failed: !m_impl->hasOneRef(). StringView does not own the StringImpl, it must not have the last ref.
#0 0x7f528ddb9c8e base::debug::StackTrace::StackTrace()
#1 0x7f528ddde21b logging::LogMessage::~LogMessage()
#2 0x7f528adb1fbf WTF::StringView::~StringView()
#3 0x7f528b10c549 blink::KURL::initProtocolMetadata()
#4 0x7f528b10bfd0 blink::KURL::init()
#5 0x7f528820e7a4 blink::Referrer::Referrer()
#6 0x7f528821ae21 blink::RequestInit::RequestInit()
#7 0x7f5288218d39 blink::Request::create()
#8 0x7f5287f63409 blink::V8Request::constructorCallback()
#9 0x7f528b719734 v8::internal::FunctionCallbackArguments::Call()
#10 0x7f528b7d51cf v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#11 0x7f528b7d4613 v8::internal::Builtin_Impl_HandleApiCall()
#12 0x3aaaa768426e <unknown>

Apparently it's a KURL problem rather than Fetch API.

Raising to P1. This seems to be a recent regression and is showing up in runs of the WebGL conformance tests per  Issue 596622 :

https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyLwsSBUZsYWtlIiR3ZWJnbF9jb25mb3JtYW5jZV90ZXN0cyAod2l0aCBwYXRjaCkM

All of the recent failures of WebglConformance_conformance_context_context_release_with_workers are this bug:

WebglConformance_conformance_context_context_release_with_workers (gpu_tests.webgl_conformance_integration_test.WebGLConformanceIntegrationTest) ... [11405:775:1219/004229.169200:FATAL:StringView.cpp(19)] Check failed: !m_impl->hasOneRef(). StringView does not own the StringImpl, it must not have the last ref.
0 Chromium Framework 0x0000000109484853 _ZN4base5debug10StackTraceC1Ev + 19
1 Chromium Framework 0x00000001094a8c37 _ZN7logging10LogMessageD2Ev + 71
2 Chromium Framework 0x000000010adddf95 _ZN3WTF10StringViewD2Ev + 181
3 Chromium Framework 0x000000010ce87820 _ZN5blink4KURL20initProtocolMetadataEv + 464
4 Chromium Framework 0x000000010ce87c85 _ZN5blink4KURLC1ERKN3WTF12AtomicStringERKN3url6ParsedEb + 69
5 Chromium Framework 0x000000010ccb258f _ZNK5blink6WebURLcvNS_4KURLEEv + 47
6 Chromium Framework 0x000000010ccb4e4b _ZN5blink14WebURLResponse6setURLERKNS_6WebURLE + 27
7 Chromium Framework 0x000000010cc0faac _ZN7content16WebURLLoaderImpl19PopulateURLResponseERK4GURLRKNS_20ResourceResponseInfoEPN5blink14WebURLResponseEb + 124
8 Chromium Framework 0x000000010cc1341f _ZN7content16WebURLLoaderImpl7Context18OnReceivedResponseERKNS_20ResourceResponseInfoE + 351
9 Chromium Framework 0x000000010cc14977 _ZN7content16WebURLLoaderImpl15RequestPeerImpl18OnReceivedResponseERKNS_20ResourceResponseInfoE + 119
10 Chromium Framework 0x000000010cbe3c6e _ZN7content18ResourceDispatcher18OnReceivedResponseEiRKNS_20ResourceResponseHeadE + 590
11 Chromium Framework 0x000000010cbe679f _ZN3IPC8MessageTI33ResourceMsg_ReceivedResponse_MetaNSt3__15tupleIJiN7content20ResourceResponseHeadEEEEvE8DispatchINS4_18ResourceDispatcherES9_vMS9_FviRKS5_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ + 175
12 Chromium Framework 0x000000010cbe3539 _ZN7content18ResourceDispatcher15DispatchMessageERKN3IPC7MessageE + 233
13 Chromium Framework 0x000000010cbe2c90 _ZN7content18ResourceDispatcher17OnMessageReceivedERKN3IPC7MessageE + 688
14 Chromium Framework 0x000000010cbe9092 _ZN7content24ResourceSchedulingFilter15DispatchMessageERKN3IPC7MessageE + 162
15 Chromium Framework 0x000000010cbe943b _ZN4base8internal13FunctorTraitsIMN7content24ResourceSchedulingFilterEFvRKN3IPC7MessageEEvE6InvokeIRKNS_7WeakPtrIS3_EEJS7_EEEvS9_OT_DpOT0_ + 155
16 Chromium Framework 0x0000000109484f71 _ZN4base5debug13TaskAnnotator7RunTaskEPKcPNS_11PendingTaskE + 225
17 Chromium Framework 0x000000010ce1e802 _ZN5blink9scheduler16TaskQueueManager24ProcessTaskFromWorkQueueEPNS0_8internal9WorkQueueEbPNS0_7LazyNowE + 1506
18 Chromium Framework 0x000000010ce1c6e1 _ZN5blink9scheduler16TaskQueueManager6DoWorkEb + 1121
19 Chromium Framework 0x000000010ce206ec _ZN4base8internal13FunctorTraitsIMN5blink9scheduler16TaskQueueManagerEFvbEvE6InvokeIRKNS_7WeakPtrIS4_EEJRKbEEEvS6_OT_DpOT0_ + 156
20 Chromium Framework 0x0000000109484f71 _ZN4base5debug13TaskAnnotator7RunTaskEPKcPNS_11PendingTaskE + 225
21 Chromium Framework 0x00000001094c1189 _ZN4base11MessageLoop7RunTaskEPNS_11PendingTaskE + 425
22 Chromium Framework 0x00000001094c154c _ZN4base11MessageLoop21DeferOrRunPendingTaskENS_11PendingTaskE + 44
23 Chromium Framework 0x00000001094c1a43 _ZN4base11MessageLoop6DoWorkEv + 467
24 Chromium Framework 0x00000001094c59a0 _ZN4base24MessagePumpCFRunLoopBase7RunWorkEv + 48
25 Chromium Framework 0x00000001094aa0aa _ZN4base3mac15CallWithEHFrameEU13block_pointerFvvE + 10
26 Chromium Framework 0x00000001094c5374 _ZN4base24MessagePumpCFRunLoopBase13RunWorkSourceEPv + 68

Let's please investigate and fix this ASAP to make these tests reliably green again. Thanks.

Can we figure out a blame range here?

Long shot but could it be csharrison's https://codereview.chromium.org/2537323002 and related changes?

I think falken@ could be right, but I the patch in question is
https://codereview.chromium.org/2463703002/, not the WebURL change.

Let's revert my patch in the short term. I am OOO so don't have much time to investigate, unfortunately, but a quick look through the code leaves me stumped. The StringView is only taking reference from the KURLs m_string, so there should be one ref from the owning KURL...

+esprehn am I missing something?

If anyone can reliably repro I would gladly appreciate it :)

This usually means the KURL was destroyed before the StringView, StringView is like StringPiece, it doesn't own the string.

The stacktrace is during KURL::init construction, right after we've initialized m_string in the KURL, so I don't think this is a use-after-free from using StringView.

Hmm yeah not sure, fwiw m_protocol = AtomicString(protocol.toString()); is actually constructing two strings, you want .toAtomicString() but that shouldn't cause this bug.

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

commit 8885747f52b034575336f97e354fa8043f6c1b9f
Author: csharrison <csharrison@chromium.org>
Date: Tue Dec 20 06:03:03 2016

Revert of Optimize KURL protocols (patchset #9 id:160001 of https://codereview.chromium.org/2463703002/ )

Reason for revert:
Causing DCHECK flakes.  crbug.com/674388 

Original issue's description:
> Optimize KURL protocols
>
> This patch optimizes KURL::protocol and KURL::protocolIs by keeping
> an AtomicString m_protocol on KURL. This reduces string allocations
> throughout the code using KURL::protocol().
>
> This also fixes an inconsistency with KURL::protocolIs that will return
> true for invalid URLs.
>
> BUG=348655
>
> Committed: https://crrev.com/775abc2d7c903f191f7b24f8b299ebabbea3f624
> Cr-Commit-Position: refs/heads/master@{#438197}

TBR=esprehn@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=348655, 674388 

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

[modify] https://crrev.com/8885747f52b034575336f97e354fa8043f6c1b9f/third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp
[modify] https://crrev.com/8885747f52b034575336f97e354fa8043f6c1b9f/third_party/WebKit/Source/platform/weborigin/KURL.cpp
[modify] https://crrev.com/8885747f52b034575336f97e354fa8043f6c1b9f/third_party/WebKit/Source/platform/weborigin/KURL.h
[modify] https://crrev.com/8885747f52b034575336f97e354fa8043f6c1b9f/third_party/WebKit/Source/platform/weborigin/KURLTest.cpp
[modify] https://crrev.com/8885747f52b034575336f97e354fa8043f6c1b9f/third_party/WebKit/Source/wtf/text/AtomicString.h
[modify] https://crrev.com/8885747f52b034575336f97e354fa8043f6c1b9f/third_party/WebKit/Source/wtf/text/StringStatics.cpp

Any news if the revert helped? FYI I wasn't able to repro using the commands in the original report and comment #1 before the revert.

The webgl_conformance_tests flakiness dashboard today looks full of systematic bot errors where tons of steps are failing.

I also couldn't tell from the flake dashboard.

Note that the stack trace might look very different if the revert didn't end up helping, as the DCHECK will never be called (no StringView in the old code).

Sorry, I forgot to mention dcheck_always_on was turned on in my release build.
I can't repro by running 100 times after the revert.

Yeah I have dcheck on always too, weird. Were you able to repro consistently by running 100 times before the revert? If so, maybe we can call this bug fixed.

A race like that makes me think the patch is causing us to ref and deref on different threads. You might try reenabling the thread checker in String. We really need to fix the paint code and turn that back on.

I will investigate tomorrow. I do not think we should mark this as fixed until we've understood the underlying issue.

If what esprehn@ says is true (and I think it is likely), this bug is *not* fixed at the root cause, we just effectively deleted the DCHECK.

I have confirmed that the string in question is *not* m_string from KURL, but the global WTF::httpsAtom.

The static atoms are safe to use from any thread, and they will not be deleted when reference count drops to 0. I think the StringView DCHECK is wrong, and should instead be DCHECK(!m_impl->hasOneRef() || m_impl->isStatic()).

Oh, and just to clarify, we implicitly make a StringView from the httpsAtom for operator== (protocol == WTF::httpsAtom).

Oh yeah that's my bad in the assert, it should be changed as you describe.

Cool, I've sent you a patch to review.

BTW for posterity, I was imprecise in #18. Static strings never have ref count drop to 0, even if they are deref'd with ref count 1.

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

commit 8b89825da13786897274a69663ea06e1c41da9e7
Author: csharrison <csharrison@chromium.org>
Date: Thu Dec 22 23:47:24 2016

Allow StringView to have the sole reference to a static string

Static strings are thread safe and can safely have their ref counts
floor at 1, so StringView should not crash if it happens to do that.

BUG= 674388 

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

[modify] https://crrev.com/8b89825da13786897274a69663ea06e1c41da9e7/third_party/WebKit/Source/wtf/text/StringView.cpp

I believe we've found the root cause of this issue, so marking as fixed.

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

commit 6a44155f151b212fa9f99acd9226505ec42e78ab
Author: csharrison <csharrison@chromium.org>
Date: Thu Jan 05 05:04:32 2017

Reland "Optimize KURL protocols"

This reverts commit 8885747f52b034575336f97e354fa8043f6c1b9f, which
reverted 775abc2d7c903f191f7b24f8b299ebabbea3f624 (reviewed at
https://codereview.chromium.org/2463703002/)

The change was triggering a bad DCHECK in ~StringView, which
has been amended in the dependent CL.

BUG=348655, 674388 

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

[modify] https://crrev.com/6a44155f151b212fa9f99acd9226505ec42e78ab/third_party/WebKit/Source/core/loader/MixedContentCheckerTest.cpp
[modify] https://crrev.com/6a44155f151b212fa9f99acd9226505ec42e78ab/third_party/WebKit/Source/platform/weborigin/KURL.cpp
[modify] https://crrev.com/6a44155f151b212fa9f99acd9226505ec42e78ab/third_party/WebKit/Source/platform/weborigin/KURL.h
[modify] https://crrev.com/6a44155f151b212fa9f99acd9226505ec42e78ab/third_party/WebKit/Source/platform/weborigin/KURLTest.cpp
[modify] https://crrev.com/6a44155f151b212fa9f99acd9226505ec42e78ab/third_party/WebKit/Source/wtf/text/AtomicString.h
[modify] https://crrev.com/6a44155f151b212fa9f99acd9226505ec42e78ab/third_party/WebKit/Source/wtf/text/StringStatics.cpp