Closed Bug 1425296 Opened 4 years ago Closed 4 years ago

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


(Core :: Networking, defect, P2)




Tracking Status
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed


(Reporter: mayhemer, Assigned: mayhemer)




(1 file, 1 obsolete file)

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.
Assignee: nobody → honzab.moz
Priority: -- → P2
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.
Summary: Very long blocking of tracking requests due to a tailing bug: move call to RequestContext.BeginLoad() from nsDocument to nsHTMLDocument → Very long blocking of tracking requests, request context can switch to before DOMContentLoaded state with an XHR.
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #8937157 - Flags: review?(bzbarsky)
Comment on attachment 8937157 [details] [diff] [review]

>+    if (mIsContentDocument) {

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

r=me if you write a reasonable commit message here.  Comment 2 is a good start.
Attachment #8937157 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Pushed by
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
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Can this ride with 59, or do we need to uplift (with the holidays we're getting close to the end of beta58)?
Flags: needinfo?(honzab.moz)
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.
It can be critical, yes.  I wanted to uplift this, will ask for a+.
Flags: needinfo?(honzab.moz)
Comment on attachment 8937480 [details] [diff] [review]

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
Attachment #8937480 - Flags: approval-mozilla-beta?
Comment on attachment 8937480 [details] [diff] [review]

fix bad regression from 57, beta58+
Attachment #8937480 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.