1348050 - Mark channels (xhr/fetch/.src=) created by scripts handling direct user interaction as urgent-start

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Honza Bambas (:mayhemer)]

The idea is to e.g. bring an image fancybox to the user ASAP after a click.  The urgent-start classification will tell the network requests scheduler to perform it with the highest priority and also ignoring any parallelism or overall connection limits.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Ideally this bit of meta-data would get properly passed through the nsIChannel to the Request object when we trigger a FetchEvent in a service worker.  That way we could properly pass it through when the SW is doing:

  respondWith(fetch(evt.request))

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

As discussion with Shianyow and Tom, this is needed for quantum networking with the expected timeframe by FF56, and Tom is going to help. According to the quick investigation by Tom, this looks doable now and not heavily impacted by the SW redesign. :bkelly, please advise if there's something missing. Thank you.

Comment 3

[Tom Tung [:tt, :ttung]]

I reckon that I should mark the request as urgent-start if a request is created after user interaction event. Besides, the request is not handled by a worker.

For setting flag, I can refer [1] to set urgent-start to channel. Moreover, I need to make sure the flag is passing through SW and redirected request.

Currently, I found EventStateManager::IsHandlingUserInput()[2] may be the function to decide the request is triggered due to user interaction event.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1348053#c7

#include "nsIClassOfService.h"
...
nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel));
if (cos) {
  cos->AddClassFlags(nsIClassOfService::UrgentStart);
}

[2] http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.cpp#3856

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Tom Tung [:tt] from comment #3)
> I reckon that I should mark the request as urgent-start if a request is
> created after user interaction event. 

Yes.

> Besides, the request is not handled by
> a worker.

I can't say.  You are probably right.  The purpose of urgent-start is to get visual feedback (that something is happening) and optimally also the desired content's first bytes ASAP.  Background work probably doesn't belong to it, if I understand correctly.

> 
> For setting flag, I can refer [1] to set urgent-start to channel. Moreover,
> I need to make sure the flag is passing through SW and redirected request.

Very good point!  I found that redirected channels don't inherit it.  Filed a bug, you don't need to care, it will be fixed.

According SW, it's more a question for :jdm or :bkelly.

> 
> Currently, I found EventStateManager::IsHandlingUserInput()[2] may be the
> function to decide the request is triggered due to user interaction event.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1348053#c7
> 
> #include "nsIClassOfService.h"
> ...
> nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(channel));
> if (cos) {
>   cos->AddClassFlags(nsIClassOfService::UrgentStart);
> }
> 
> [2]
> http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.
> cpp#3856

Hmm.. the default for this is 1000ms since the last interaction.  I'm afraid that scripts that may do something soon during a page load, e.g from DOMContentLoaded, could flood the urgent-start queues and limits.  Such network requests should be handled as "normal" parallel loads, IMO.  But if it becomes too complicated to find out something is use interaction triggered, let's try this way.

Comment 5

[Tom Tung [:tt, :ttung]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
Thanks for the quick feedback!

> I can't say.  You are probably right.  The purpose of urgent-start is to get
> visual feedback (that something is happening) and optimally also the desired
> content's first bytes ASAP.  Background work probably doesn't belong to it,
> if I understand correctly.

I'll double check it before starting to implement.

> According SW, it's more a question for :jdm or :bkelly.

Thanks, I'll ask them when I have questions for SW.
 
> Hmm.. the default for this is 1000ms since the last interaction.  I'm afraid

It returns false when sUserInputEventDepth gets zero which is decreased by StopHandlingUserInput() as well (after notifying event listener).

> that scripts that may do something soon during a page load, e.g from
> DOMContentLoaded, could flood the urgent-start queues and limits.  Such
> network requests should be handled as "normal" parallel loads, IMO.  But if
> it becomes too complicated to find out something is use interaction
> triggered, let's try this way.

Hmm, I don't think we treat DOMContentLoaded as user input event, but I'm not pretty sure about that. If we don't treat it as user input event, these network requests are handled as normal parallel loads. I'll check it out. 

I have an another question about promise and setTimeout. Should I mark the request inside them as urgent-start if promise or setTimeout are triggered by user interaction event? I have that question because these requests are suspended in original scenario.

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #5)
> that scripts that may do something soon during a page load, e.g from
> DOMContentLoaded, could flood the urgent-start queues and limits.  Such

Oh, I realized you meant one second is too long and it makes other requests become urgent-start. If "sUserInputEventDepth" get zero, we should be able to avoid that.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Tom Tung [:tt] from comment #6)
> (In reply to Tom Tung [:tt] from comment #5)
> > that scripts that may do something soon during a page load, e.g from
> > DOMContentLoaded, could flood the urgent-start queues and limits.  Such
> 
> Oh, I realized you meant one second is too long and it makes other requests
> become urgent-start. If "sUserInputEventDepth" get zero, we should be able
> to avoid that.

Yes, exactly.  Then this is the right API to use, thanks!

Comment 8

[Tom Tung [:tt, :ttung]]

Hi Honza,

I still have a question about creating a request on another tasks or microtasks (e.g. setTimeout, promise). I know we should mark the request which is triggered by user input event directly. But, what about those request which is triggered indirectly (requests in the tasks created by user input triggering task)?

For example:
```
addEventListener('click', function () {
  fetch("foo1.com"); // Should mark as urgent-start
  setTimeout(_ => fetch("foo2.com"), 0); // Will be execute after fetch("foo3.com"). Not sure if marking this as urgent-start or not
  fetch("foo3.com"); // Should mark as urgent-start
});
```
Should I need to mark the request insides setTimeout/promise as urgentStart since they will be executed later?

If we want to do that, I may need to adding urgent start flag on tasks and their children tasks which are trigger initially by user input event.

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

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

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 2: Create test for fetch and xhr to ensure we set the urgent start flag only when they are triggered by user input event.

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

Comment 11

[Tom Tung [:tt, :ttung]]

Current progress:

- Part 1: Mark the flag on fetch and xhr first since they are easier to mark the urgentStart flag compare to .src.
  - ToDo items: Should I explicitly check if the caller is worker? When the request is handled by worker, they should not found the any user input event in event state manager. But, I'm not sure about that, so I will check it out.

- Part 2: test for fetch and xhr
  - ToDo items: 1. not sure how to check the channel's flag while sending a fetch or xhr. 
                2. test for worker

Since we have many elements which have src attribute. I will do it later. I'll have Part 3 for src and Part 4 for the test of src. 
I put the results for .src I found so far:

grep -r "attribute DOMString src;" dom/webidl
dom/webidl/HTMLEmbedElement.webidl:           attribute DOMString src;
dom/webidl/HTMLFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLIFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLImageElement.webidl:           attribute DOMString src;
dom/webidl/HTMLInputElement.webidl:           attribute DOMString src;
dom/webidl/HTMLMediaElement.webidl:           attribute DOMString src;
dom/webidl/HTMLScriptElement.webidl:  attribute DOMString src;
dom/webidl/HTMLSourceElement.webidl:           attribute DOMString src;
dom/webidl/HTMLTrackElement.webidl:  attribute DOMString src;
dom/webidl/SpeechGrammar.webidl:    attribute DOMString src;

grep -rn "GetAttr" dom | grep "nsGkAtoms::src"
dom/base/nsFrameLoader.cpp:818:                  mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::srcdoc,
dom/base/nsFrameLoader.cpp:2552:    mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::src, aURI);
dom/base/nsObjectLoadingContent.cpp:1716:    thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::src, uriStr);
dom/html/HTMLImageElement.cpp:986:    if (!GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLImageElement.cpp:1228:  if (!aSourceNode->GetAttr(kNameSpaceID_None, nsGkAtoms::srcset, srcset)) {
dom/html/HTMLImageElement.cpp:1252:    if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src) && !src.IsEmpty()) {
dom/html/HTMLInputElement.cpp:4980:      GetAttr(kNameSpaceID_None, nsGkAtoms::src, uri) &&
dom/html/HTMLInputElement.cpp:5190:    if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLMediaElement.cpp:1947:  } else if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLMediaElement.cpp:2243:    if (!child->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLScriptElement.cpp:281:  if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/html/HTMLTrackElement.cpp:281:  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
dom/media/TextTrack.cpp:357:    if (!(mTrackElement->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src))) {
dom/xul/templates/nsXULTreeBuilder.cpp:661:        cell->GetAttr(kNameSpaceID_None, nsGkAtoms::src, raw);

Comment 12

[Tom Tung [:tt, :ttung]]

Updated the patch for testing fetch & XHR:
- Finished XHR part
  - Create an XHR via XPCOM interface since I can get its channel from this interface. Thus I can check if the urgent start flag is set in cos flag.
  - Move the test to chrome mochi test since accessing xhr.channel needs chrome privilege.
  - Test it in nine situations(123 * abc): 
      send an XHR 
        1. without event triggering 2. with user input event triggering 3. with non-user input event triggering.
        a. in the original task     b. in an another micro-task (promise)  c. in an another task (setTimeout)

Comment 13

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b46d15e94d24a7b997aa2ed1ff022b93c7e65aa

Comment 14

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

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

Comment 15

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860274 [details]
Bug 1348050 - Part 2: Create test for fetch and xhr to ensure we set the urgent start flag only when they are triggered by user input event.

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

Comment 16

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

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

Comment 17

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

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

Comment 18

[Tom Tung [:tt, :ttung]]

Hi Honza,

I've finished fetch and xhr part. I'll send a review request to baku as well after getting your r+.

In the Part 1, I checked the EventStateManager if there is triggered by user input event when we just set the fetch request and it is about to send xhr request. Then store it as a flag and check its value after the channel is created.

In the Part 2, I wrote a test for verify the behavior.
Note: I'm not sure about whether should I take care about the created micro-task (promise test) and task (setTimeout). If you think we should set the flag as well when they are triggered by user input events, I will file a follow-up bug for it.

I'll update the patch for src and its test when I finish it.

Comment 19

[Tom Tung [:tt, :ttung]]

Clear ni? since I put it in the patch (Part2) for review

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review137746

::: dom/fetch/FetchDriver.cpp:260
(Diff revision 3)
>    }
>  #endif
>    chan->SetNotificationCallbacks(this);
>  
> +  nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(chan));
> +  if (cos && mUseUrgentStartForChannel) {

can you explain why you can't use EventStateManager::IsHandlingUserInput() here?

::: dom/xhr/XMLHttpRequestMainThread.cpp:2658
(Diff revision 3)
>    // deadlock where server generation of CSS/JS requires an XHR signal.
>    nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(mChannel));
>    if (cos) {
>      cos->AddClassFlags(nsIClassOfService::Unblocked);
> +
> +    if (mUseUrgentStartForChannel) {

here as well?

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

https://reviewboard.mozilla.org/r/134272/#review137748

::: dom/base/test/test_urgent_start.html:1
(Diff revision 1)
> +<!DOCTYPE HTML>

I prefer to have a comment at the top of a test that describes what the test is testing and what steps it takes so that one doesn't need to study the code to understand it.

::: dom/base/test/test_urgent_start.html:9
(Diff revision 1)
> +  <title>Test for urgentStart(fetch and xhr)</title>
> +  <script type="application/javascript"
> +          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet"
> +        type="text/css"
> +        href="chrome://mochikit/content/tests/SimpleTest/test.css"?>

some fishy char at the tail

Comment 22

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review137746

> can you explain why you can't use EventStateManager::IsHandlingUserInput() here?

I don't have a strong reason for it. I'll move EventStateManager::IsHandlingUserInput() to here.

I wanted to check EventStateManager::IsHandlingUserInput() only when we have windows(not a worker). Thus, I added it ineside the check for windows[1] so that I don't need to check it again.

[1] http://searchfox.org/mozilla-central/source/dom/fetch/Fetch.cpp#342-379

> here as well?

Same as above. I'll move it to here. Thanks!

Comment 23

[Tom Tung [:tt, :ttung]]

Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

https://reviewboard.mozilla.org/r/134272/#review137748

> I prefer to have a comment at the top of a test that describes what the test is testing and what steps it takes so that one doesn't need to study the code to understand it.

Sure!

> some fishy char at the tail

Oops, I somehow miss that... I'll correct it. Thanks!

Comment 24

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132290/diff/3-4/

Comment 25

[Tom Tung [:tt, :ttung]]

Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

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

Comment 26

[Tom Tung [:tt, :ttung]]

Addressed comment and sent r? to :baku.

Hi Honza,

If you see there are still somethings wrong, please feel free to point it out. Thanks!

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138346

::: dom/xhr/XMLHttpRequestMainThread.cpp:2661
(Diff revision 4)
> +
> +    // Mark channel as urgent-start if the XHR is triggered by user input
> +    // events. Note: we should not set the channel for worker.
> +    nsCOMPtr<nsIDocument> responsibleDoc = GetDocumentIfCurrent();
> +    if (responsibleDoc &&
> +        responsibleDoc->NodePrincipal() == mPrincipal &&

Random comment, why do we need the principal check?

Comment 28

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138346

> Random comment, why do we need the principal check?

Hi Olli,

Thanks for the feedback! 

I wanted to check whehter it is initiated by user input events. Therefore, I added the document and principal check to prevent it from being called by worker. Thus, I copied the check for worker [1].

[1] http://searchfox.org/mozilla-central/source/dom/xhr/XMLHttpRequestMainThread.cpp#2473-2477

Comment 29

[Andrea Marchesini [:baku]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138766

::: dom/xhr/XMLHttpRequestMainThread.cpp:2661
(Diff revision 4)
> +
> +    // Mark channel as urgent-start if the XHR is triggered by user input
> +    // events. Note: we should not set the channel for worker.
> +    nsCOMPtr<nsIDocument> responsibleDoc = GetDocumentIfCurrent();
> +    if (responsibleDoc &&
> +        responsibleDoc->NodePrincipal() == mPrincipal &&

I don't think EventStateManager::IsHandlingUserInput() returns true if there are workers involved. if this is used by workers, this method has been called by a runnable.

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

https://reviewboard.mozilla.org/r/134272/#review138768

Comment 31

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138766

> I don't think EventStateManager::IsHandlingUserInput() returns true if there are workers involved. if this is used by workers, this method has been called by a runnable.

I see, so I'll remove the extra check for the worker (document check in FetchDriver, document and principal checks in XHR). Thanks!

Comment 32

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

https://reviewboard.mozilla.org/r/132290/#review138346

> Hi Olli,
> 
> Thanks for the feedback! 
> 
> I wanted to check whehter it is initiated by user input events. Therefore, I added the document and principal check to prevent it from being called by worker. Thus, I copied the check for worker [1].
> 
> [1] http://searchfox.org/mozilla-central/source/dom/xhr/XMLHttpRequestMainThread.cpp#2473-2477

I'll remvoe it based on Andrea's comment.

Comment 33

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fc51934cd5a7bdae2d02cd2c648b15f8df00d23

Comment 34

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132290/diff/4-5/

Comment 35

[Tom Tung [:tt, :ttung]]

Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

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

Comment 36

[Tom Tung [:tt, :ttung]]

Hi Honza,

I greped the "attribute DOMString src;" from dom/webidl and I got:
dom/webidl/HTMLEmbedElement.webidl:           attribute DOMString src;
dom/webidl/HTMLFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLIFrameElement.webidl:           attribute DOMString src;
dom/webidl/HTMLImageElement.webidl:           attribute DOMString src;
dom/webidl/HTMLInputElement.webidl:           attribute DOMString src;
dom/webidl/HTMLMediaElement.webidl:           attribute DOMString src;
dom/webidl/HTMLScriptElement.webidl:  attribute DOMString src;
dom/webidl/HTMLSourceElement.webidl:           attribute DOMString src;
dom/webidl/HTMLTrackElement.webidl:  attribute DOMString src;
dom/webidl/SpeechGrammar.webidl:    attribute DOMString src;

I've finished marking urgent-start for HTMLEmbedElement, HTMLFrameElement, HTMLIFrameElement, HTMLImageElement, HTMLInputElement, HTMLMediaElement and HTMLScriptElement. I'll send them to review once the try goes green.

However, I think marking urgent-start for src attributes of HTMLScriptElement, HTMLTrackElement and SpeechGrammar are not necessary. 

The reason for script element is that it has its own mechanism to preload the script. As for the track element (webvtt), it usually open channel for loading txt (e.g. subtitle). For the SpeechGrammar, I think it's not related to this bug.

Thus, I won't mark urgent-start for them.
Please correct me if you have different thoughts for this.

Comment 37

[Tom Tung [:tt, :ttung]]

Attachment: Trailing whitespace.

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

Comment 38

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

This part is mainly to mark the channel as urgent-start if src related
attributes in HTMLImageElement and HTMLInputElement is set and the channel is
open due to user interaction. Unfortunately, we cannot just check the event
state just after creating channel since some loading image tasks will be queue
and execute in stable state. Thus, I store the event state in elements and
pass it to the place where create the channel.

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

Comment 39

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 4: Mark channel as urgent-start for loading media.

The part is mainly to set urgent-start for loading media when it's initiated by
user interaction. For the HTMLMedia element, it has its algorithm to pre-load
the media and users will get feedback once they play the media. Thus, I only
set urgent-start when the media element has autoplay attribute since the sooner
enough resource is loaded the sooner the media will be played.

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

Comment 40

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

This part is mainly to set channel as urgent-start for loading frame/iframe.
Since we create few runnables between the place to get src(FrameLoader) and the
palce to create channel(DocShell), I chose to store the flag on FrameLoader and
pass it to DocShell.

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

Comment 41

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 5: Mark channel as urgent-start for embed element.

This patch is mainly for ObjectLoadingContent. Since there is no any runnable
between setting src and creating channel, I check urgent-start just after
creating channel.

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

Comment 42

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1348050 - Part 6: Expand the original test to verify src attribute.

Since there are some mechanism to prevent from loading multiplace resorce at a
same time for an element, I rewrote the test to isolate each subtests and make
sure they will not bother each other.

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

Comment 43

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132290/diff/5-6/

Comment 44

[Tom Tung [:tt, :ttung]]

Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134272/diff/3-4/

Comment 45

[Honza Bambas (:mayhemer)]

(In reply to Tom Tung [:tt] from comment #36)
> Hi Honza,
> 
> I greped the "attribute DOMString src;" from dom/webidl and I got:
> dom/webidl/HTMLEmbedElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLFrameElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLIFrameElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLImageElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLInputElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLMediaElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLScriptElement.webidl:  attribute DOMString src;
> dom/webidl/HTMLSourceElement.webidl:           attribute DOMString src;
> dom/webidl/HTMLTrackElement.webidl:  attribute DOMString src;
> dom/webidl/SpeechGrammar.webidl:    attribute DOMString src;
> 
> I've finished marking urgent-start for HTMLEmbedElement, HTMLFrameElement,
> HTMLIFrameElement, HTMLImageElement, HTMLInputElement, HTMLMediaElement and
> HTMLScriptElement. I'll send them to review once the try goes green.
> 
> However, I think marking urgent-start for src attributes of
> HTMLScriptElement, HTMLTrackElement and SpeechGrammar are not necessary. 

I agree.  Actually, this should only be implemented for visual impacting elements, such as img, video, audio, embed.  iframe, frame and script and definitely track are not necessary IMO.

Is SpeechGrammar something people expect to be loaded and played ASAP after a user interaction?  I don't know what SpeechGrammar is tho :)

> 
> The reason for script element is that it has its own mechanism to preload
> the script. As for the track element (webvtt), it usually open channel for
> loading txt (e.g. subtitle). For the SpeechGrammar, I think it's not related
> to this bug.
> 
> Thus, I won't mark urgent-start for them.
> Please correct me if you have different thoughts for this.

I think your decisions are OK!  This doesn't need to be perfect at the first stage.

Thanks!

Comment 46

[Andrea Marchesini [:baku]]

Comment on attachment 8865301 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for embed element.

https://reviewboard.mozilla.org/r/136940/#review140022

Comment 47

[Andrea Marchesini [:baku]]

Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140024

I prefer bz to review this patch. Or any other docShell peer.

Comment 48

[Andrea Marchesini [:baku]]

Comment on attachment 8865299 [details]
Bug 1348050 - Part 4: Mark channel as urgent-start for loading media.

https://reviewboard.mozilla.org/r/136936/#review140026

Comment 49

[Honza Bambas (:mayhemer)]

Comment on attachment 8865298 [details]
Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

https://reviewboard.mozilla.org/r/136934/#review140092

I can only review the bit that adds the class on the channel.  I'm not an imagelib nor a dom peer, you need to find someone else to this review fully.


r+ for only the imgLoader.cpp parts

::: dom/base/nsImageLoadingContent.cpp:661
(Diff revision 1)
>    // We keep this flag around along with the old URI even for failed requests
>    // without a live request object
>    ImageLoadType loadType = \
>      (mCurrentRequestFlags & REQUEST_IS_IMAGESET) ? eImageLoadType_Imageset
>                                                   : eImageLoadType_Normal;
> +

?

::: image/imgLoader.cpp:2284
(Diff revision 1)
>      MOZ_LOG(gImgLog, LogLevel::Debug,
>             ("[this=%p] imgLoader::LoadImage -- Created new imgRequest"
>              " [request=%p]\n", this, request.get()));
>  
> +    nsCOMPtr<nsIClassOfService> cos(do_QueryInterface(newChannel));
> +    if (cos && aUseUrgentStartForChannel) {

same question as in another patch:

why not use EventStateManager::IsHandlingUserInput() just here?  

I don't say the approach you use is wrong, it just needs some explanation why it has to be captured so much above and carried down here via a member and number of arguments.  it's a lot of code changes and when we add a new place where src is handled, it will need an update to set the urgent start thing.

Comment 50

[Honza Bambas (:mayhemer)]

Comment on attachment 8865299 [details]
Bug 1348050 - Part 4: Mark channel as urgent-start for loading media.

https://reviewboard.mozilla.org/r/136936/#review140096

I can't review this.  You need someone from the dom or media team to do it.

Comment 51

[Honza Bambas (:mayhemer)]

Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140098

bug 1348044 handles this.  and please see https://bugzilla.mozilla.org/show_bug.cgi?id=1348044#c4.  we don't want this for iframes/frames.  I *think* that for nsDocShell::OnLinkClickSync the [isTopLevelDoc && mIsActive || (mLoadType & (LOAD_CMD_NORMAL | LOAD_CMD_HISTORY))] cond will eval to true.

IMO, this patch is a WONTFIX.

Comment 52

[Honza Bambas (:mayhemer)]

Comment on attachment 8865301 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for embed element.

https://reviewboard.mozilla.org/r/136940/#review140106

You need a dom peer to r this.

Comment 53

[Honza Bambas (:mayhemer)]

Comment on attachment 8865302 [details]
Bug 1348050 - Part 6: Expand the original test to verify src attribute.

https://reviewboard.mozilla.org/r/136942/#review140112

Comment 54

[Tom Tung [:tt, :ttung]]

Comment on attachment 8865298 [details]
Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

https://reviewboard.mozilla.org/r/136934/#review140092

Thanks!

> ?

Sorry, I somehow added this empty line and forgot to remove it...

> same question as in another patch:
> 
> why not use EventStateManager::IsHandlingUserInput() just here?  
> 
> I don't say the approach you use is wrong, it just needs some explanation why it has to be captured so much above and carried down here via a member and number of arguments.  it's a lot of code changes and when we add a new place where src is handled, it will need an update to set the urgent start thing.

Rather than checking the IsHandlingUserInput just after creating a channel, I checked IsHandlingUserInput and store it when we store the src URI. Then, I passed the flag to the function where we create the channel.

The reason it that when we load a image in responsive mode[1], we will queue the loading task and run it in stable state[2]. I was afriad the task might be queue'd more than one second which will make the user input check invaild [3]. 

Thus, I chose to check the flag when we set the src attribute or something else that may trigger a image load to prevent it from running more than one second. 

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#420
[2] http://searchfox.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#916-920
[3] http://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#309

Comment 55

[Tom Tung [:tt, :ttung]]

Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140098

I see. I'll remove this patch. Thanks!

Comment 56

[Tom Tung [:tt, :ttung]]

Comment on attachment 8865300 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for loading frame/iframe.

https://reviewboard.mozilla.org/r/136938/#review140024

I'll remove this patch due to Honza's comment.

Comment 57

[Andrea Marchesini [:baku]]

Comment on attachment 8865302 [details]
Bug 1348050 - Part 6: Expand the original test to verify src attribute.

https://reviewboard.mozilla.org/r/136942/#review140498

Comment 58

[Andrea Marchesini [:baku]]

Comment on attachment 8865298 [details]
Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

https://reviewboard.mozilla.org/r/136934/#review140502

Comment 59

[Tom Tung [:tt, :ttung]]

Comment on attachment 8860273 [details]
Bug 1348050 - Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132290/diff/6-7/

Comment 60

[Tom Tung [:tt, :ttung]]

Comment on attachment 8862352 [details]
Bug 1348050 - Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134272/diff/4-5/

Comment 61

[Tom Tung [:tt, :ttung]]

Comment on attachment 8865298 [details]
Bug 1348050 - Part 3: Mark channel as urgent-start for loading image.

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

Comment 62

[Tom Tung [:tt, :ttung]]

Comment on attachment 8865299 [details]
Bug 1348050 - Part 4: Mark channel as urgent-start for loading media.

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

Comment 63

[Tom Tung [:tt, :ttung]]

Comment on attachment 8865301 [details]
Bug 1348050 - Part 5: Mark channel as urgent-start for embed element.

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

Comment 64

[Tom Tung [:tt, :ttung]]

Comment on attachment 8865302 [details]
Bug 1348050 - Part 6: Expand the original test to verify src attribute.

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

Comment 65

[Tom Tung [:tt, :ttung]]

(In reply to Andrea Marchesini [:baku] from comment #58)
(In reply to Honza Bambas (:mayhemer) from comment #53)

Thanks for reviews!!

Comment 66

[Honza Bambas (:mayhemer)]

(In reply to Tom Tung [:tt] from comment #54)
> The reason it that when we load a image in responsive mode[1], we will queue
> the loading task and run it in stable state[2]. I was afriad the task might
> be queue'd more than one second which will make the user input check invaild
> [3]. 
> 

Aha.  But if the task is queued for so long time it probably doesn't make any sense to try to urge the network request then.  Anyway, the patch's been written, let's leave it. 

Hmm..  I absolutely don't know this code, but should we give src assignment coming from a user input (for images) some higher priority?  definitely for a different bug!

Thanks.

Comment 67

[Tom Tung [:tt, :ttung]]

(In reply to Honza Bambas (:mayhemer) from comment #66)
> Aha.  But if the task is queued for so long time it probably doesn't make
> any sense to try to urge the network request then.  Anyway, the patch's been
> written, let's leave it. 
> 
> Hmm..  I absolutely don't know this code, but should we give src assignment
> coming from a user input (for images) some higher priority?  definitely for
> a different bug!
> 
> Thanks.

Sorry, I find I forgot to publish the another comment in review board. Still try to get used into it...

There is an stronger reason for it. Since it will create another runnable(task) and run it in stable state, event has gone when we check it. Thus, we need to check it before we create the runnable and pass it when we start to run that runnable. 

I meant that if we only check event for image load just after creating channel, it will always get false in responsive mode.

Comment 68

[Tom Tung [:tt, :ttung]]

(In reply to Honza Bambas (:mayhemer) from comment #66)
> Hmm..  I absolutely don't know this code, but should we give src assignment
> coming from a user input (for images) some higher priority?  definitely for
> a different bug!

Hmm, I thought the ugrent-start is the highest one. If we have a higher one, I reckon we could do that, but I don't have a strong opinion for it.

Comment 69

[Honza Bambas (:mayhemer)]

(In reply to Tom Tung [:tt] from comment #68)
> (In reply to Honza Bambas (:mayhemer) from comment #66)
> > Hmm..  I absolutely don't know this code, but should we give src assignment
> > coming from a user input (for images) some higher priority?  definitely for
> > a different bug!
> 
> Hmm, I thought the ugrent-start is the highest one. If we have a higher one,
> I reckon we could do that, but I don't have a strong opinion for it.

I might express myself in a confusing way.  'urgent-start' is telling the network scheduler to put the request (represented by the channel) on the wire ASAP, no need for any more fiddle with priorities.  

What I suggest above is to change also the code in dom/image handling.  There seems to be a delay (that can be more than a second?) introduced as part of the "responsive mode" handling before we even create the network channel.

Comment 70

[Tom Tung [:tt, :ttung]]

(In reply to Honza Bambas (:mayhemer) from comment #69)
> I might express myself in a confusing way.  'urgent-start' is telling the
> network scheduler to put the request (represented by the channel) on the
> wire ASAP, no need for any more fiddle with priorities.  

Understand!

> What I suggest above is to change also the code in dom/image handling. 
> There seems to be a delay (that can be more than a second?) introduced as
> part of the "responsive mode" handling before we even create the network
> channel.

I think I might mislead you... Sorry for that :(

In general, it should not delay so long especially Q-DOM marking the foreground task as a higher priority. 

I think the reason why we put this into another microtask/task to awit another metastable/stable state is that we don't want to occpuy main thread too much time and we can do some other microtask/task between them. Besides, it is written in the spec, so the other broswer should handle them similar to this as well.

Anyway, I just tried to implement a way that can handle all the situations which I can imagine even it rarely happens. It doesn't mean that we usually have a delay more than one second for loading an image.

Comment 71

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24e81e3c9712495e16a27fc5471534fe8d068a66&selectedJob=97620695

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e11ea0ec07a4
Part 1: Mark channel as urgent-start when the Fetch and XHR are triggered by user input events. r=baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/60b9bc8fd85a
Part 2: Create a chrome mochitest for Fetch and XHR to ensure we set the urgent-start flag only when they are triggered by user input events. r=baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/3011f497829a
Part 3: Mark channel as urgent-start for loading image. r=baku,mayhemer
https://hg.mozilla.org/integration/autoland/rev/98c06ef34ca8
Part 4: Mark channel as urgent-start for loading media. r=baku
https://hg.mozilla.org/integration/autoland/rev/d2d736b78d1e
Part 5: Mark channel as urgent-start for embed element. r=baku
https://hg.mozilla.org/integration/autoland/rev/05ff3de940c7
Part 6: Expand the original test to verify src attribute. r=baku,mayhemer

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e11ea0ec07a4
https://hg.mozilla.org/mozilla-central/rev/60b9bc8fd85a
https://hg.mozilla.org/mozilla-central/rev/3011f497829a
https://hg.mozilla.org/mozilla-central/rev/98c06ef34ca8
https://hg.mozilla.org/mozilla-central/rev/d2d736b78d1e
https://hg.mozilla.org/mozilla-central/rev/05ff3de940c7

Comment 74

[Shian-Yow Wu [:swu]]

Thank you Tom!

Comment 75

[Tom Tung [:tt, :ttung]]

(In reply to Shian-Yow Wu [:swu] from comment #74)
> Thank you Tom!

np! Thanks for review and feedback, Honza, Andrea! :)