1425296 - Very long blocking of tracking requests, request context can switch to before DOMContentLoaded state with an XHR.

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Honza Bambas (:mayhemer)]

Discovered during investigation of bug 1423373.  An XHR request within the same loadgroup triggers that code as well and reverts the associated rc to a pre-DOMContentLoaded state.  This can prolong untail.

Comment 1

[Honza Bambas (:mayhemer)]

Adding this to HTLM document only is probably not the best solution since an HTML document creation is not indication of page load beginning all the time.

I'll leave the code where is but let trigger only when mIsContentDocument is true for the document.  This works for top level and iframe loads but doesn't pass for XHR with xml response.  Interestingly though, we don't create an html doc when xhr response is html.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

So, RequestContext->BeginLoad() drops the RequestContext's internal mAfterDOMContentLoaded flag to false.  

The place I want to call this should be the beginning of load of top level or iframe document, but not a SVG subresource or XHR with an XML response.  What is in the patch seems to be the best place, but feel free to suggest a better one.

A RequestContext instance is 1-1 with a load group and the load group MUST be the one for which the document is being loaded in.

The RequestContext.mAfterDOMContentLoaded has a very strong effect on Tracking Request tailing (bug 1358060) scheduling.  When the document load phase is between load begin and domcontentloaded we delay async tracker script requests by order of hundreds of milliseconds (usually several seconds).  after domcontentloaded the delay drops to just 20 milliseconds which opens the floods.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8937157 [details] [diff] [review]
v1

>+    if (mIsContentDocument) {

Please use "if (IsContentDocument()) {".

r=me if you write a reasonable commit message here.  Comment 2 is a good start.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1

thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8797318866d9e112137fa5c947aa8a4306ddfd2f

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b3bd97ecf45
Don't let RequestContext's load state be reset to 'before-DOMContentLoaded' by any sub-requested document within the load group to prevent long delays of tailed (tracking) requests. r=bz

Comment 6

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3b3bd97ecf45

Comment 7

[Julien Cristau [:jcristau]]

Can this ride with 59, or do we need to uplift (with the holidays we're getting close to the end of beta58)?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

This is a pretty critical problem, isn't it?  Also AIUI we got an extra week in January before the next merge when the extra holiday time was added to the calendar.

Comment 9

[Honza Bambas (:mayhemer)]

It can be critical, yes.  I wanted to uplift this, will ask for a+.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8937480 [details] [diff] [review]
v1.1

Approval Request Comment
[Feature/Bug causing the regression]: bug 1358060 
[User impact if declined]: page may not finish loading (spin forever), tracker scripts may never load and never execute (including e.g. stat counters etc..)
[Is this code covered by automated tests?]: no (will file a bug)
[Has the fix been verified in Nightly?]: yes, but locally only
[Needs manual test from QE? If yes, steps to reproduce]: this is quite timing critical, so it's hard to provide a manual test that would be reliable ; just in case, see this bug URL in comment 0 to attempt to reproduce the never ending load
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: the code is not critical, doesn't disallow loads or anything ; if this breaks (we don't call the BeginLoad function because of the condition), we only disallow an optimization whose effect is relatively small
[String changes made/needed]: none

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8937480 [details] [diff] [review]
v1.1

fix bad regression from 57, beta58+

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c4bbf9fb645