1414745 - nsBrowserStatusFilter::OnStateChange delivers asymmetric numbers of STATE_START & STATE_STOP

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Samael Wang [:freesamael] (away for now)]

I noticed this problem when working on bug 1402689. It seems nsBrowserStatusFilter::OnStateChange tends to deliver more STATE_STOP than STATE_START (but sometimes reversely, I found a case in bug 463210 comment 5), and that's likely an existing problem for many years.

Now that we applied nsBrowserStatusFilter on both parent and child processes, so on parent side mFinishedRequests often exceeds mTotalRequests [1], and make the filter be broken frequently.

I don't see a specific bug it causes at this moment, but if consumers of the filter care about STATE_REQUEST we should fix it.

[1] http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp#159,169,203

Comment 1

[Samael Wang [:freesamael] (away for now)]

It seems browser.js & tabbrowser only care about STATE_IS_NETWORK, and we don't seem to use the fake progress generated by the statusfilter either (the value is incorrect with current implementation).
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp#164,174-175

Maybe we can simply filter out all state changes except `(STATE_IS_NETWORK & STATE_START) || (STATE_IS_NETWORK & STATE_STOP)`?

Comment 2

[Samael Wang [:freesamael] (away for now)]

Looks like fennec already did so (filter out everything except STATE_IS_NETWORK):
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/mobile/android/chrome/content/browser.js#4356-4357

Comment 3

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1414745 - Filter out everything except STATE_IS_NETWORK in nsBrowserStatusFilter::OnStateChange.

The mFinishedRequests / mTotalRequests counters in nsBrowserStatusFilter has
been known buggy for around a decade. With e10s we double filtered in both
parent and child processes which makes it worse.

There are a few problems with the counters / filter:
1. The ResetMembers() were invoked on each network request starts, which means
   a iframe document loading followed by the root document loading would also
   reset the members incorrectly.
2. The filter for non-network request was incorrect. The basic idea seems to be
   that if `mFinishedRequests == mTotalRequests` and `!isLoadingDocument`, it
   should be the STOP request right after document loading finishes, and that
   STOP should be delivered so listener would get symmetric numbers of START /
   STOP of STATE_IS_REQUEST. However some requests such as imgRequest can start
   after document loading finishes, in this case the START would be filtered
   out (since mFinishedRequests != mTotalRequests), but STOP would be delivered
   to the listener. It's the reason that nsBrowserStatusFilter tend to deliver
   much more STATE_STOP than STATE_START.
3. When applying the filter on both parent & child side, the above issues often
   make the mFinishedRequests / mFinishedRequests be unmatched on parent side,
   eseentially filtered out most non-network requests and make the
   progressChange based on the counters useless.

Firefox no longer shows the ratio of progressChange on the UI (and the number
is incorrect anyway with current nsBrowserStatusFilter), and Fennec's progress
bar is based on some predefined constants [1] which doesn't rely on
progressChange either, so it not necessary to keep calculating a progress
number with request counters.

In addition, it seems tabbrowser & browser.js mostly only care about
STATE_IS_NETWORK, and Fennec has already filtered out everything else [2], it
should be safe to only pass STATE_IS_NETWORK to the listener, and we get the
benefit of reducing unused IPC messages.

[1] https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/mobile/android/base/java/org/mozilla/gecko/Tab.java#111-115
[2] https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/mobile/android/chrome/content/browser.js#4356-4357

Review commit: https://reviewboard.mozilla.org/r/200690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200690/

Comment 4

[Samael Wang [:freesamael] (away for now)]

Hi Mike,

It may look a bit too aggressive, but given that we've already filtered out most non-network STATE_IS_REQUEST and don't have correct number for the fake onProgressChange, I think removing the mFinishedRequests / mTotalRequests counters and only delivering STATE_IS_NETWORK reflect our use case now.

Since you're familiar with both gecko and frontend, I'd like to hear your feedback on this change.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8929369 [details]
Bug 1414745 - Filter out everything except STATE_IS_NETWORK in nsBrowserStatusFilter::OnStateChange.

https://reviewboard.mozilla.org/r/200690/#review206358

Thanks! Code seems great - just a few notes. See below.

::: commit-message-40df5:25
(Diff revision 1)
> +Firefox no longer shows the ratio of progressChange on the UI (and the number
> +is incorrect anyway with current nsBrowserStatusFilter), and Fennec's progress
> +bar is based on some predefined constants [1] which doesn't rely on
> +progressChange either, so it not necessary to keep calculating a progress
> +number with request counters.
> +
> +In addition, it seems tabbrowser & browser.js mostly only care about
> +STATE_IS_NETWORK, and Fennec has already filtered out everything else [2], it
> +should be safe to only pass STATE_IS_NETWORK to the listener, and we get the
> +benefit of reducing unused IPC messages.

If you wouldn't mind, could you please send a message to dev-platform about this change? Or, alternatively, maybe find a way of communicating the change to the TB and SM communities?

I ask, because I see a non-zero amount of usage of @mozilla.org/appshell/component/browser-status-filter;1 in comm-central, and I'm not certain what assumptions they make about its behaviour.

::: toolkit/components/statusfilter/nsBrowserStatusFilter.cpp:153
(Diff revision 1)
>  {
>      if (!mListener)
>          return NS_OK;
>  
>      if (aStateFlags & STATE_START) {
> -        if (aStateFlags & STATE_IS_NETWORK) {
> +        // Reset members on begining of document loading, but we don't want

Typo: "begining" -> "beginning"

::: toolkit/components/statusfilter/nsBrowserStatusFilter.cpp
(Diff revision 1)
>  void
>  nsBrowserStatusFilter::ResetMembers()
>  {
> -    mTotalRequests = 0;
> -    mFinishedRequests = 0;
> -    mUseRealProgressFlag = false;

Seems we're explicitly not resetting `mIsLoadingDocument` back to `false` here, likely because `ResetMembers()` is only called in one place, and that one place now sets `mIsLoadingDocument` to `true` immediately after calling it.

Would you mind adding a comment in ResetMembers about how we're explicitly not resetting mIsLoadingDocument to false?

Comment 6

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8929369 [details]
Bug 1414745 - Filter out everything except STATE_IS_NETWORK in nsBrowserStatusFilter::OnStateChange.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200690/diff/1-2/

Comment 7

[Samael Wang [:freesamael] (away for now)]

Sent a mail to dev-platform. I'll wait for a day or 2 before landing in case someone found the change is inappropriate.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6dd2a65b4c3e
Filter out everything except STATE_IS_NETWORK in nsBrowserStatusFilter::OnStateChange. r=mconley

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6dd2a65b4c3e