Reland [Resource Timing] Expose redirect timing information

Reland [Resource Timing] Expose redirect timing information

Contributed by pan.deng@intel.com

BUG=252066
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154675

[Pan, 2013-06-21 15:22:28]

https://codereview.chromium.org/16950017/diff/1/Source/core/page/Performance.cpp
File Source/core/page/Performance.cpp (right):

https://codereview.chromium.org/16950017/diff/1/Source/core/page/Performance....
Source/core/page/Performance.cpp:229: double lastRedirectEndTime =
lastRedirectTiming->receiveHeadersEnd;
James and Tony

Resource Timing spec says last byte header end time from network as
"redirectEnd", so here it should come from
lastRedirectTiming->receiveHeadersEnd.
While, as ResourceLoadTiming is filled from chrome part, as we went through
network cache, app cache, HSTS redirect case, I feel it is risk to assume chrome
always feed blink a filled ResourceLoadTiming, so I prefer to record the
redirect receive time from blink as a fallback, when lastRedirectTiming here is
a none pointer, use timeticks recorded in blink, it would be better than crash.
The cons is losing the way to know who breaks the assumption.
any idea?

thanks
Pan

[James Simonsen, 2013-06-22 00:26:32]

https://codereview.chromium.org/16950017/diff/1/Source/core/page/Performance.cpp
File Source/core/page/Performance.cpp (right):

https://codereview.chromium.org/16950017/diff/1/Source/core/page/Performance....
Source/core/page/Performance.cpp:229: double lastRedirectEndTime =
lastRedirectTiming->receiveHeadersEnd;
On 2013/06/21 15:22:28, Pan wrote:
> James and Tony
> 
> Resource Timing spec says last byte header end time from network as
> "redirectEnd", so here it should come from
> lastRedirectTiming->receiveHeadersEnd.
> While, as ResourceLoadTiming is filled from chrome part, as we went through
> network cache, app cache, HSTS redirect case, I feel it is risk to assume
chrome
> always feed blink a filled ResourceLoadTiming, so I prefer to record the
> redirect receive time from blink as a fallback, when lastRedirectTiming here
is
> a none pointer, use timeticks recorded in blink, it would be better than
crash.
> The cons is losing the way to know who breaks the assumption.
> any idea?
> 
> thanks
> Pan

Personally, I'd rather fix all the cases in net/ and keep the ASSERT where it
was. That way nobody would be able to regress it in Chrome, because they'd cause
tests to start failing. That's better than having it broken and have Blink cover
up the problem.

[Pan, 2013-06-28 16:16:19]

> Personally, I'd rather fix all the cases in net/ and keep the ASSERT where it
> was. That way nobody would be able to regress it in Chrome, because they'd
cause
> tests to start failing. That's better than having it broken and have Blink
cover
> up the problem.

As we fix HSTS timing issue in chrome network, 1st ASSERT failure during page
cycler test that addressed in
http://code.google.com/p/chromium/issues/detail?id=252066#c3 has gone.
For the 2nd Linux ASAN issue, unfortunately, it is not reproduced in my local
machine, dpranke has filed an issue to set up a webkit linux asan trybot, not
done yet (https://code.google.com/p/chromium/issues/detail?id=252983).
I suggest we reland this CL then go through tests(as some tests is not covered
by trybot), survive or find other issue to fix. or other methodology?

[James Simonsen, 2013-07-09 22:05:44]

On 2013/06/28 16:16:19, Pan wrote:
> > Personally, I'd rather fix all the cases in net/ and keep the ASSERT where
it
> > was. That way nobody would be able to regress it in Chrome, because they'd
> cause
> > tests to start failing. That's better than having it broken and have Blink
> cover
> > up the problem.
> 
> As we fix HSTS timing issue in chrome network, 1st ASSERT failure during page
> cycler test that addressed in
> http://code.google.com/p/chromium/issues/detail?id=252066#c3 has gone.
> For the 2nd Linux ASAN issue, unfortunately, it is not reproduced in my local
> machine, dpranke has filed an issue to set up a webkit linux asan trybot, not
> done yet (https://code.google.com/p/chromium/issues/detail?id=252983).
> I suggest we reland this CL then go through tests(as some tests is not covered
> by trybot), survive or find other issue to fix. or other methodology?

Sorry, I was on vacation last week. It looks like the trybot has been added. Can
you try it out and see if it reproduces?

[Pan, 2013-07-09 22:34:56]

On 2013/07/09 22:05:44, James Simonsen wrote:
> On 2013/06/28 16:16:19, Pan wrote:
> > > Personally, I'd rather fix all the cases in net/ and keep the ASSERT where
> it
> > > was. That way nobody would be able to regress it in Chrome, because they'd
> > cause
> > > tests to start failing. That's better than having it broken and have Blink
> > cover
> > > up the problem.
> > 
> > As we fix HSTS timing issue in chrome network, 1st ASSERT failure during
page
> > cycler test that addressed in
> > http://code.google.com/p/chromium/issues/detail?id=252066#c3 has gone.
> > For the 2nd Linux ASAN issue, unfortunately, it is not reproduced in my
local
> > machine, dpranke has filed an issue to set up a webkit linux asan trybot,
not
> > done yet (https://code.google.com/p/chromium/issues/detail?id=252983).
> > I suggest we reland this CL then go through tests(as some tests is not
covered
> > by trybot), survive or find other issue to fix. or other methodology?
> 
> Sorry, I was on vacation last week. It looks like the trybot has been added.
Can
> you try it out and see if it reproduces?

It is not reproduced, see
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_asan/bui...,
is it the right way to land the patch again and see what happens :)

thanks
Pan

[James Simonsen, 2013-07-09 22:41:41]

On 2013/07/09 22:34:56, Pan wrote:
> On 2013/07/09 22:05:44, James Simonsen wrote:
> > On 2013/06/28 16:16:19, Pan wrote:
> > > > Personally, I'd rather fix all the cases in net/ and keep the ASSERT
where
> > it
> > > > was. That way nobody would be able to regress it in Chrome, because
they'd
> > > cause
> > > > tests to start failing. That's better than having it broken and have
Blink
> > > cover
> > > > up the problem.
> > > 
> > > As we fix HSTS timing issue in chrome network, 1st ASSERT failure during
> page
> > > cycler test that addressed in
> > > http://code.google.com/p/chromium/issues/detail?id=252066#c3 has gone.
> > > For the 2nd Linux ASAN issue, unfortunately, it is not reproduced in my
> local
> > > machine, dpranke has filed an issue to set up a webkit linux asan trybot,
> not
> > > done yet (https://code.google.com/p/chromium/issues/detail?id=252983).
> > > I suggest we reland this CL then go through tests(as some tests is not
> covered
> > > by trybot), survive or find other issue to fix. or other methodology?
> > 
> > Sorry, I was on vacation last week. It looks like the trybot has been added.
> Can
> > you try it out and see if it reproduces?
> 
> It is not reproduced, see
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_asan/bui...,
> is it the right way to land the patch again and see what happens :)
> 
> thanks
> Pan

I'm not sure it's the "right way," but that's what I'd do. I wonder if we have a
way to skip a test only on ASAN.

I'm curious why it failed on that one bot last time though. Maybe it's a race?
Or something's uninitialized? Do you remember how often it failed last time?

[Pan, 2013-07-09 23:01:22]

On 2013/07/09 22:41:41, James Simonsen wrote:
> On 2013/07/09 22:34:56, Pan wrote:
> > On 2013/07/09 22:05:44, James Simonsen wrote:
> > > On 2013/06/28 16:16:19, Pan wrote:
> > > > > Personally, I'd rather fix all the cases in net/ and keep the ASSERT
> where
> > > it
> > > > > was. That way nobody would be able to regress it in Chrome, because
> they'd
> > > > cause
> > > > > tests to start failing. That's better than having it broken and have
> Blink
> > > > cover
> > > > > up the problem.
> > > > 
> > > > As we fix HSTS timing issue in chrome network, 1st ASSERT failure during
> > page
> > > > cycler test that addressed in
> > > > http://code.google.com/p/chromium/issues/detail?id=252066#c3 has gone.
> > > > For the 2nd Linux ASAN issue, unfortunately, it is not reproduced in my
> > local
> > > > machine, dpranke has filed an issue to set up a webkit linux asan
trybot,
> > not
> > > > done yet (https://code.google.com/p/chromium/issues/detail?id=252983).
> > > > I suggest we reland this CL then go through tests(as some tests is not
> > covered
> > > > by trybot), survive or find other issue to fix. or other methodology?
> > > 
> > > Sorry, I was on vacation last week. It looks like the trybot has been
added.
> > Can
> > > you try it out and see if it reproduces?
> > 
> > It is not reproduced, see
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_asan/bui...,
> > is it the right way to land the patch again and see what happens :)
> > 
> > thanks
> > Pan
> 
> I'm not sure it's the "right way," but that's what I'd do. I wonder if we have
a
> way to skip a test only on ASAN.
> 
> I'm curious why it failed on that one bot last time though. Maybe it's a race?
> Or something's uninitialized? Do you remember how often it failed last time?

Yep, Let's have more tests on ASAN bot. I try several times locally all passed,
even hundred iterations to run the
http/tests/xmlhttprequest/workers/referer.html
Last time dpranke reported the failure is about
http/tests/xmlhttprequest/workers/referer.html. unfortunately the log is
cleared... IIRC, it is 
1, thread T0 create worker thread 
2, worker thread alloc thread global data D1(including
CachedResourceRequestInitiators there) when request a resource, 
3 thread T0 release D1, 
4, worker thread access D1's address again, and cause a failure.

thanks
Pan

[James Simonsen, 2013-07-09 23:33:27]

On 2013/07/09 23:01:22, Pan wrote:
> On 2013/07/09 22:41:41, James Simonsen wrote:
> > On 2013/07/09 22:34:56, Pan wrote:
> > > On 2013/07/09 22:05:44, James Simonsen wrote:
> > > > On 2013/06/28 16:16:19, Pan wrote:
> > > > > > Personally, I'd rather fix all the cases in net/ and keep the ASSERT
> > where
> > > > it
> > > > > > was. That way nobody would be able to regress it in Chrome, because
> > they'd
> > > > > cause
> > > > > > tests to start failing. That's better than having it broken and have
> > Blink
> > > > > cover
> > > > > > up the problem.
> > > > > 
> > > > > As we fix HSTS timing issue in chrome network, 1st ASSERT failure
during
> > > page
> > > > > cycler test that addressed in
> > > > > http://code.google.com/p/chromium/issues/detail?id=252066#c3 has gone.
> > > > > For the 2nd Linux ASAN issue, unfortunately, it is not reproduced in
my
> > > local
> > > > > machine, dpranke has filed an issue to set up a webkit linux asan
> trybot,
> > > not
> > > > > done yet (https://code.google.com/p/chromium/issues/detail?id=252983).
> > > > > I suggest we reland this CL then go through tests(as some tests is not
> > > covered
> > > > > by trybot), survive or find other issue to fix. or other methodology?
> > > > 
> > > > Sorry, I was on vacation last week. It looks like the trybot has been
> added.
> > > Can
> > > > you try it out and see if it reproduces?
> > > 
> > > It is not reproduced, see
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_asan/bui...,
> > > is it the right way to land the patch again and see what happens :)
> > > 
> > > thanks
> > > Pan
> > 
> > I'm not sure it's the "right way," but that's what I'd do. I wonder if we
have
> a
> > way to skip a test only on ASAN.
> > 
> > I'm curious why it failed on that one bot last time though. Maybe it's a
race?
> > Or something's uninitialized? Do you remember how often it failed last time?
> 
> Yep, Let's have more tests on ASAN bot. I try several times locally all
passed,
> even hundred iterations to run the
> http/tests/xmlhttprequest/workers/referer.html
> Last time dpranke reported the failure is about
> http/tests/xmlhttprequest/workers/referer.html. unfortunately the log is
> cleared... IIRC, it is 
> 1, thread T0 create worker thread 
> 2, worker thread alloc thread global data D1(including
> CachedResourceRequestInitiators there) when request a resource, 
> 3 thread T0 release D1, 
> 4, worker thread access D1's address again, and cause a failure.
> 
> thanks
> Pan

This sounds something like
https://code.google.com/p/chromium/issues/detail?id=237801. Does that look
familiar? I can't reproduce it either.

I don't think we should even report Resource Timing from workers. Maybe if we
fixed that, we'd fix both of these issues.

[Pan, 2013-07-09 23:50:55]

> This sounds something like
> https://code.google.com/p/chromium/issues/detail?id=237801. Does that look
> familiar? I can't reproduce it either.
> 
> I don't think we should even report Resource Timing from workers. Maybe if we
> fixed that, we'd fix both of these issues.

I don't have the permission to access #237801, may be I need to be in the CC
list.
I didn't see worker thing in Resource Timing spec, if it is an item of Resource
Timing 2, I think we should disable here.
What I'm puzzled is why T0 release worker's ThreadGlobalData, that may be a
general issue rather than specific one of Resource Timing,(even without this
patch, worker will go to addResourceTiming).

thanks
Pan

[James Simonsen, 2013-07-10 00:04:46]

On 2013/07/09 23:50:55, Pan wrote:
> > This sounds something like
> > https://code.google.com/p/chromium/issues/detail?id=237801. Does that look
> > familiar? I can't reproduce it either.
> > 
> > I don't think we should even report Resource Timing from workers. Maybe if
we
> > fixed that, we'd fix both of these issues.
> 
> I don't have the permission to access #237801, may be I need to be in the CC
> list.
> I didn't see worker thing in Resource Timing spec, if it is an item of
Resource
> Timing 2, I think we should disable here.
> What I'm puzzled is why T0 release worker's ThreadGlobalData, that may be a
> general issue rather than specific one of Resource Timing,(even without this
> patch, worker will go to addResourceTiming).
> 
> thanks
> Pan

Oh, it's a crash bug. I guess those are hidden. Anyway, it looks like the same
root cause to me. Lemme see if I can hack up a CL that doesn't call
addResourceTiming from inside workers...

[Pan, 2013-07-16 10:45:46]

@tonyg, @japhet, @simonjam

The dependencies were resolved(hsts, disable worker report), can we reland this
patch now? nothing changed except rebase.

thanks!
Pan

[James Simonsen, 2013-07-16 19:05:12]

lgtm

(not an owner)

[Pan, 2013-07-17 23:36:22]

@tonyg and japhet,

your idea? :)

thanks
Pan

[Nate Chapin, 2013-07-18 16:03:40]

core/loader/ changes LGTM with one nit.

https://codereview.chromium.org/16950017/diff/19001/Source/core/loader/Resour...
File Source/core/loader/ResourceLoadNotifier.cpp (left):

https://codereview.chromium.org/16950017/diff/19001/Source/core/loader/Resour...
Source/core/loader/ResourceLoadNotifier.cpp:54:
request.setReportLoadTiming(true);
Why is this moving?

[Pan, 2013-07-18 22:31:25]

On 2013/07/18 16:03:40, Nate Chapin wrote:
> core/loader/ changes LGTM with one nit.
> 
>
https://codereview.chromium.org/16950017/diff/19001/Source/core/loader/Resour...
> File Source/core/loader/ResourceLoadNotifier.cpp (left):
> 
>
https://codereview.chromium.org/16950017/diff/19001/Source/core/loader/Resour...
> Source/core/loader/ResourceLoadNotifier.cpp:54:
> request.setReportLoadTiming(true);
> Why is this moving?

it was moved to ResourceLoader, intention is to report loadTiming for all
requests, including xhr preflight.
right?

thanks
Pan

[Pan, 2013-07-19 21:42:15]

@tonyg, any more comments?
thanks,
Pan

[tonyg, 2013-07-20 19:58:24]

lgtm

I'm still a little worried that we don't understand what the failure was before.
But I guess give it a try. Worst case scenario we just have to rollback again.

[commit-bot: I haz the power, 2013-07-22 00:39:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pan.deng@intel.com/16950017/19001

[commit-bot: I haz the power, 2013-07-22 00:39:19]

Failed to apply patch for Source/core/loader/ResourceLoader.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/loader/ResourceLoader.cpp
  Hunk #1 FAILED at 277.
  Hunk #2 FAILED at 288.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/core/loader/ResourceLoader.cpp.rej

Patch:       Source/core/loader/ResourceLoader.cpp
Index: Source/core/loader/ResourceLoader.cpp
diff --git a/Source/core/loader/ResourceLoader.cpp
b/Source/core/loader/ResourceLoader.cpp
index
ca965c86ab5665d34f32fc2b776d0554d2e8ec9e..a5ad3fa9c4ae9960c453a7ac870e882804e51d72
100644
--- a/Source/core/loader/ResourceLoader.cpp
+++ b/Source/core/loader/ResourceLoader.cpp
@@ -277,6 +277,7 @@ void ResourceLoader::willSendRequest(ResourceHandle*,
ResourceRequest& request,
             cancel();
             return;
         }
+        m_documentLoader->cachedResourceLoader()->redirectReceived(m_resource,
redirectResponse);
         m_resource->willSendRequest(request, redirectResponse);
     }
 
@@ -288,6 +289,8 @@ void ResourceLoader::willSendRequest(ResourceHandle*,
ResourceRequest& request,
     else
         InspectorInstrumentation::willSendRequest(m_frame.get(),
m_resource->identifier(), m_documentLoader.get(), request, redirectResponse,
m_options.initiatorInfo);
 
+    request.setReportLoadTiming(true);
+
     m_request = request;
 
     if (request.isNull())

[commit-bot: I haz the power, 2013-07-22 14:45:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pan.deng@intel.com/16950017/40001

[commit-bot: I haz the power, 2013-07-22 15:21:39]

Retried try job too often on linux_layout_rel for step(s) webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-07-22 18:50:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pan.deng@intel.com/16950017/40001

[commit-bot: I haz the power, 2013-07-22 19:35:34]

Change committed as 154675