Make sure that a channel's priority would not be modified once it has been set by nsChannelClassifier

RESOLVED WONTFIX

Status

()

Core
Networking
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: kershaw, Assigned: kershaw)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox53 affected)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
See bug 1141814 comment #49.

We have to avoid NetworkPrioritizer setting the channel's priority if the priority is already set as PRIORITY_LOWEST by nsChannelClassifier.
(Assignee)

Updated

2 years ago
Blocks: 1141814
Whiteboard: [necko-next]
Kershaw, does NetworkPrioritizer increases/decreases the priority of channels (I assume within a tab) or does it rewrite it?  I'd like to start synthetic testing of bug 1141814 in reality and this seems like a breaker.
Assignee: nobody → kechang
Flags: needinfo?(kechang)
Whiteboard: [necko-next] → [necko-active]
(Assignee)

Comment 2

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #1)
> Kershaw, does NetworkPrioritizer increases/decreases the priority of
> channels (I assume within a tab) or does it rewrite it?  I'd like to start
> synthetic testing of bug 1141814 in reality and this seems like a breaker.

NetworkPrioritizer actually increases/decreases the priority, but the value of priority it adjusts every time is 10 [1].

My current idea is quite simple now: to add a new attribute in nsISupportsPriority or nsIHttpChannelInternal to indicate that the priority should be fixed and not is not allowed to change. This new attribute will be set by nsChannelClassifier.

What do you think about this idea?

[1] http://searchfox.org/mozilla-central/rev/e8835f52eff29772a57dca7bcc86a9a312a23729/browser/modules/NetworkPrioritizer.jsm#33
Flags: needinfo?(kechang) → needinfo?(honzab.moz)
(Assignee)

Comment 3

2 years ago
Created attachment 8817448 [details] [diff] [review]
Prevent changing the priority of a tracking channel

Honza,
Could you please take a look at this patch? Thanks.
Flags: needinfo?(honzab.moz)
Attachment #8817448 - Flags: review?(honzab.moz)
If this is just about making the test reliable, then we may do a "relative" test.  Just take priority of e.g. the top level channel and a tracker channel and compare them.  The tracker should be higher (the actual internal priority value) by "N", or at least be "<=" in general.  That can be a good start.

I'm not happy about the patch if there is no other reason than reliability of the test.
(Assignee)

Comment 5

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #4)
> If this is just about making the test reliable, then we may do a "relative"
> test.  Just take priority of e.g. the top level channel and a tracker
> channel and compare them.  The tracker should be higher (the actual internal
> priority value) by "N", or at least be "<=" in general.  That can be a good
> start.
> 
> I'm not happy about the patch if there is no other reason than reliability
> of the test.

This patch is not all about the reliability of the test. It's about making sure that once the channel has been annotated as in tp list, its priority will be stuck to PRIORITY_LOWEST.

In the real world, it is possible that NetworkPrioritizer adjusts the priority of tracking channels due to the switch between tabs. So, I think we still might need a patch to avoid the priority being changed by NetworkPrioritizer.
(Assignee)

Comment 6

2 years ago
Comment on attachment 8817448 [details] [diff] [review]
Prevent changing the priority of a tracking channel

Review of attachment 8817448 [details] [diff] [review]:
-----------------------------------------------------------------

Since we might want to add a new load flag bit in bug 1319359 to indicate this is a tracking channel, it is not necessary to add a new attribute in nsIHttpChannelInternal.

Cancel the review for now.
Attachment #8817448 - Flags: review?(honzab.moz)
(Assignee)

Comment 7

2 years ago
Honza,

Since bug 1170190 has landed, I think we could add a check in nsHttpChannel::SetPriority like:

if (mIsTrackingResource) {
  return NS_OK;
}

The objective is to prevent the priority being changed if it's a tracking channel. It's possible that the tracking channel's priority will be changed by switching the active tab. It's not all about the reliability of the test.

What do yo think about this idea?
(Assignee)

Comment 8

2 years ago
Please see comment#7.
Flags: needinfo?(honzab.moz)
(In reply to Kershaw Chang [:kershaw] from comment #7)
> Honza,
> 
> Since bug 1170190 has landed, I think we could add a check in
> nsHttpChannel::SetPriority like:
> 
> if (mIsTrackingResource) {
>   return NS_OK;
> }
> 
> The objective is to prevent the priority being changed if it's a tracking
> channel. It's possible that the tracking channel's priority will be changed
> by switching the active tab. 

Yeah, but it will NOT change relatively to all other requests in the same load group.  Which makes it a non-problem from my POV.

> It's not all about the reliability of the test.
> 
> What do yo think about this idea?

Honestly, the priority changes made by the JS prioritizer will go away when we finish the B/C slot functionality as part of the CDP project.  The absolute priority number will remain the same, just http connection manager will see them differently based on information about the originating tab bound to channels (transactions) and the currently active tab (bug 1312782 and around).

Hence, if you want to add something like this only temporarily to make the test stable, OK.  But file a bug to remove it when we no longer need it.

Also, in the test, you can work with difference of the load group's priority from some expected default absolute value.  If you just subtract the load group's priority difference from the channel's priority, you probably get the absolute value you are expecting.  Makes sense?
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 10

2 years ago
(In reply to Honza Bambas (:mayhemer) from comment #9)
> (In reply to Kershaw Chang [:kershaw] from comment #7)
> > Honza,
> > 
> > Since bug 1170190 has landed, I think we could add a check in
> > nsHttpChannel::SetPriority like:
> > 
> > if (mIsTrackingResource) {
> >   return NS_OK;
> > }
> > 
> > The objective is to prevent the priority being changed if it's a tracking
> > channel. It's possible that the tracking channel's priority will be changed
> > by switching the active tab. 
> 
> Yeah, but it will NOT change relatively to all other requests in the same
> load group.  Which makes it a non-problem from my POV.
> 
> > It's not all about the reliability of the test.
> > 
> > What do yo think about this idea?
> 
> Honestly, the priority changes made by the JS prioritizer will go away when
> we finish the B/C slot functionality as part of the CDP project.  The
> absolute priority number will remain the same, just http connection manager
> will see them differently based on information about the originating tab
> bound to channels (transactions) and the currently active tab (bug 1312782
> and around).
> 
Yes, I agree with you that we don't need this bug if NetworkPrioritizer will be removed when we complete bug 1312782. It seems silly to add some code and then remove in the future.

Since the current unit test is quite reliable now, I'll close this bug as WONTFIX.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.