handling scripts can block UI

• Component: DOM
• Status: RESOLVED FIXED

[Yong Li]

Created attachment 33349 [details]
the patch

CachedScript calls client->notifyFinished() immediately if the script is already in cache, and it also does that when all resource data comes.

In the case that the user goes back/forward to a page that contains many JS files, HMTLTokenizer will decode and execute those JS files at one time. This can affect UI responsiveness.

The attached patch contains code to delay sending notifications to cliensts of CachedScript by timer.

It helps improving responsiveness on WinMob

(Probably this is not a problem at all for desktop browsers?)

[Eric Seidel (no email)]

Comment on attachment 33349 [details]
the patch

For example.  When a script tag finishes loading, I think js can get a "on load" event sent.  If that's true, then that event will be sent before execution instead of after execution in your port.  I would have to look to confirm if there is an onload for <script> tags.

Seems 120 vs. 0 should be a constant defined with an #ifdef at one location in the file.

When would this ever be hit?
5     if (!m_clients.contains(c))
 166         return;
I would think that would be an error.

Style:
80             if (!m_clientsToNotify.isEmpty()) {
 181                 m_notifyTimer.startOneShot(0);
 182             } else {
 183                 m_decodedDataDeletionTimer.startOneShot(120);
 184             }


The change looks technically correct.  But I'm not sure this will function correctly from a web page's perspective.

The change need more comment as to why this will be OK from a web page's perspective.

[George Staikos]

We definitely need test cases for this.  Good point re: onload.

[Yong Li]

Well, CachedScript originally supports async mode, because it's loading network resource. It's sync mode only when the resource happens to exist in the cache.

The patch actually uses async mode for both cases.

There was a bug introduced by that, which is fixed in DocumentLoader::isLoadingInAPISense(). isLoadingInAPISense() returned false if tokenizer is not processing source and there's no pending request for that document. The patch fixes that by checking Document::parsing().

I don't think it can introduce onload() event problem. If there is a problem, it must have existed in original code, because old code uses async mode, too.

[Yong Li]

Created attachment 34857 [details]
test case

this is a simple test case

[Yong Li]

Created attachment 34859 [details]
modified for coding style

only changed CACHED_SCRIPT_DELAY_NOTIFICATION into ENABLE(CACHED_SCRIPT_DELAY_NOTIFICATION)

[Oliver Hunt]

Am i worried about functional correctness of this patch as it is deliberately allowing incorrect behaviour -- if i have 
<script>
..do stuff..
</script>
<script>
...do more stuff...
</script>

It should not be possible for the dom to change in any way between these two script tags, but what you are doing allows the state to change

[Yong Li]

(In reply to comment #6)
> Am i worried about functional correctness of this patch as it is deliberately
> allowing incorrect behaviour -- if i have 
> <script>
> ..do stuff..
> </script>
> <script>
> ...do more stuff...
> </script>
> 
> It should not be possible for the dom to change in any way between these two
> script tags, but what you are doing allows the state to change

Thanks for commenting. But could you please make it clear what state changes?

The order won't change. HTMLTokenizer::notifyFinished() always handles the first one in the pending script list.

Even without this patch, the scripts can be pending, too.

Inline scripts won't be affected. It's only for this case:

<script src =... ></script>

[Antti Koivisto]

Comment on attachment 34859 [details]
modified for coding style

I think CachedScript is wrong place to do this. It is likely have all kinds of unforeseen side effect. If you want to chop processing to smaller pieces on scripts that is probably better done in the tokenizer level which already has logic and timers to do processing in reasonable sized chunks. HTMLTokenizer::notifyFinished(CachedResource*) might be the place to start looking.

Also I think it should be possible to implement this in a manner that works well unversally on all platforms and does not require #ifdeffing. The code rot is pretty certain for this kind of code otherwise.

[Yong Li]

(In reply to comment #8)
> (From update of attachment 34859 [details])
> I think CachedScript is wrong place to do this. It is likely have all kinds of
> unforeseen side effect. If you want to chop processing to smaller pieces on
> scripts that is probably better done in the tokenizer level which already has
> logic and timers to do processing in reasonable sized chunks.
> HTMLTokenizer::notifyFinished(CachedResource*) might be the place to start
> looking.
> 
> Also I think it should be possible to implement this in a manner that works
> well unversally on all platforms and does not require #ifdeffing. The code rot
> is pretty certain for this kind of code otherwise.

Antti, I have to say your idea is better. This can be done in HTMLTokenzier.

Sigh. I should talk to you earlier.

The patch is rejected, but the bug should still open until it's fixed in HTMLTokenizer, right?

[Yael]

Created attachment 44738 [details]
Patch. 

Implement Antti's proposal to break the script execution in HTMLTokenizer.cpp.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44738 [details] without any errors.

[Antti Koivisto]

Comment on attachment 44738 [details]
Patch. 

Looks good, r=me

[WebKit Commit Bot]

Comment on attachment 44738 [details]
Patch. 

Clearing flags on attachment: 44738

Committed r52091: <http://trac.webkit.org/changeset/52091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.