Captive portal detection shouldn't block first paint

RESOLVED FIXED in Firefox 55

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
a month ago
16 days ago

People

(Reporter: florian, Assigned: nhnt11)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance][qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
In this cold startup profile, captive portal detection pays the expensive cost of initializing NSS on the main thread, as it's doing the first https request: https://perf-html.io/public/3e380602698ab6d41de775ae1a3402eceef39362/calltree/?callTreeFilters=prefix-01MnyG5yG6yH0yH1yH2yH3yH4zQvzR0zE4zE5zR2zR3zE8zR4zR5zR6zEhzEiBKszU1zElzPozU2zU3zU4yHfCM7Cx2KczU5zUiBFpN8Cx2KczU6zUiBFpN8Cx2KczUf&range=0.0000_5.3842&thread=0

In the short term, we should move captive portal initialization to after the first paint.

A better long term solution will be to start captive portal detection during startup as soon as the network can be used without causing extra overhead. We would like NSS to be initialized off main thread, and to somehow let the main thread know that it's now possible to use it.

Updated

a month ago
Whiteboard: [photon-performance][qf] → [photon-performance][qf] [triage]

Updated

24 days ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: adrian.florinescu
Whiteboard: [photon-performance][qf] [triage] → [photon-performance][qf]
Comment hidden (mozreview-request)

Updated

24 days ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Comment 2

24 days ago
Currently it appears that at startup, the CaptivePortalWatcher (browser-captivePortal.js) is the only consumer triggering a recheck. Per my investigation, commenting out the recheck call in that file results in no XHR ever being created from captivedetect.js long after the first window has loaded and even after some minor browsing. It seems that the calls in nsIOService when the network link changes or when the pref changes don't happen at startup. When I realized there were no other rechecks being triggered I didn't dig deeper.

Once the NSS-init-off-main-thread work is done, we should investigate for a better way to trigger the first recheck.
(Reporter)

Comment 3

24 days ago
mozreview-review
Comment on attachment 8873257 [details]
Bug 1367450 - Don't trigger captive portal check until delayed startup has completed.

https://reviewboard.mozilla.org/r/144712/#review148682

Looks good to me. I wonder if there's a way to test this. Currently captivedetect.js doesn't show up at all in the test I wrote in bug 1358798, so blacklisting it there before first paint wouldn't help. I assume captive portal detection is off for mochitests to avoid external network requests. I wonder if we could change this to instead check an url hosted by the mochitest server, so that the tests would see a behavior closer to what users experience. Is this something you can investigate?
Attachment #8873257 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)

Comment 5

23 days ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2b294e57559
Defer initialization of CaptivePortalWatcher to just before completion of delayed startup. r=florian
Backed out for ESLint failures.
https://hg.mozilla.org/integration/autoland/rev/768208c845b30b551928216939fb16926bd5426a

https://treeherder.mozilla.org/logviewer.html#?job_id=103702822&repo=autoland
Flags: needinfo?(nhnt11)
Comment hidden (mozreview-request)
(Assignee)

Updated

23 days ago
Flags: needinfo?(nhnt11)

Comment 8

23 days ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ff5590f1750d
Defer initialization of CaptivePortalWatcher to just before completion of delayed startup. r=florian

Comment 9

23 days ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a3bdbdd4d95a
Backed out changeset ff5590f1750d for browser_CaptivePortalWatcher.js failures.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

23 days ago
Comment on attachment 8873257 [details]
Bug 1367450 - Don't trigger captive portal check until delayed startup has completed.

Sorry about the test failures, try seems green now.

CaptivePortalWatcher attempts to open the portal tab after xul-window-visible if a portal has already been detected before the window was opened. There is a test for this.

If we move CaptivePortalWatcher into delayedStartup, xul-window-visible is a thing of the past, so it waits forever to open the tab, causing the test to fail.

Rather than messing with the mechanism of the portal tab/tests, I've simply kept the CaptivePortalWatcher.init() call where it was, and instead have moved the recheck trigger to after delayed-startup within browser-captivePortal.js, which is what we really cared about anyway.
Attachment #8873257 - Flags: review+ → review?(florian)

Updated

23 days ago
Whiteboard: [photon-performance][qf] → [photon-performance][qf:p1]
(Reporter)

Comment 12

22 days ago
mozreview-review
Comment on attachment 8873257 [details]
Bug 1367450 - Don't trigger captive portal check until delayed startup has completed.

https://reviewboard.mozilla.org/r/144712/#review149194

::: browser/base/content/browser-captivePortal.js:54
(Diff revision 4)
>    },
>  
>    init() {
> +    // We trigger a portal check after delayed startup to avoid doing a network
> +    // request before first paint.
> +    Services.obs.addObserver(this, "browser-delayed-startup-finished");

Should this only be in the cps.state == cps.UNKNOWN case? You don't want to add this observer for each new window, right?

::: browser/base/content/browser-captivePortal.js
(Diff revision 4)
>        let windows = Services.wm.getEnumerator("navigator:browser");
>        if (windows.getNext() == window && !windows.hasMoreElements()) {
>          this.ensureCaptivePortalTab();
>        }
>      }
> -

it seems you want:
  } else if (cps.state == cps.UNKNOWN) {
    this._delayedRecheckPending = true;
    Services.obs.addObserver(this, "browser-delayed-startup-finished");
  }
Attachment #8873257 - Flags: review?(florian) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 14

17 days ago
mozreview-review
Comment on attachment 8873257 [details]
Bug 1367450 - Don't trigger captive portal check until delayed startup has completed.

https://reviewboard.mozilla.org/r/144712/#review150734

Thanks!

::: browser/base/content/browser-captivePortal.js:90
(Diff revision 5)
>    },
>  
>    observe(aSubject, aTopic, aData) {
>      switch (aTopic) {
> +      case "browser-delayed-startup-finished":
> +        cps.recheckCaptivePortal();

nit: I would remove the observer and reset the _delayedRecheckPending flag before calling cps.recheckCaptivePortal() so that if it throws we don't leave an observer behind. This is a purely theoritical concern though, as the current recheckCaptivePortal implementation always returns NS_OK.
Attachment #8873257 - Flags: review?(florian) → review+
Here's a profile where this is blocking first paint for over 500ms.

https://perfht.ml/2rMQjbA
Depends on: 1366133
(Reporter)

Comment 16

17 days ago
(In reply to Mike Conley (:mconley) from comment #15)
> Here's a profile where this is blocking first paint for over 500ms.
> 
> https://perfht.ml/2rMQjbA

In that profile NSS has already been initialized by bug 1370832, and the cost you are seeing here is bug 1360164.
Comment hidden (mozreview-request)

Comment 18

16 days ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/90deb122d260
Don't trigger captive portal check until delayed startup has completed. r=florian

Comment 19

16 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90deb122d260
Status: ASSIGNED → RESOLVED
Last Resolved: 16 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

16 days ago
Iteration: --- → 55.7 - Jun 12
You need to log in before you can comment on or make changes to this bug.