Bug 27612 - handling scripts can block UI
Summary: handling scripts can block UI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-23 11:56 PDT by Yong Li
Modified: 2009-12-14 07:21 PST (History)
8 users (show)

See Also:


Attachments
the patch (7.19 KB, patch)
2009-07-23 11:56 PDT, Yong Li
eric: review-
Details | Formatted Diff | Diff
test case (983 bytes, application/zip)
2009-08-14 10:40 PDT, Yong Li
no flags Details
modified for coding style (8.81 KB, patch)
2009-08-14 10:58 PDT, Yong Li
koivisto: review-
Details | Formatted Diff | Diff
Patch. (4.93 KB, patch)
2009-12-12 07:44 PST, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2009-07-23 11:56:25 PDT
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?)
Comment 1 Eric Seidel (no email) 2009-08-06 20:31:16 PDT
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.
Comment 2 George Staikos 2009-08-06 20:37:57 PDT
We definitely need test cases for this.  Good point re: onload.
Comment 3 Yong Li 2009-08-07 07:16:25 PDT
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.
Comment 4 Yong Li 2009-08-14 10:40:11 PDT
Created attachment 34857 [details]
test case

this is a simple test case
Comment 5 Yong Li 2009-08-14 10:58:30 PDT
Created attachment 34859 [details]
modified for coding style

only changed CACHED_SCRIPT_DELAY_NOTIFICATION into ENABLE(CACHED_SCRIPT_DELAY_NOTIFICATION)
Comment 6 Oliver Hunt 2009-08-18 12:05:08 PDT
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
Comment 7 Yong Li 2009-08-18 12:10:06 PDT
(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>
Comment 8 Antti Koivisto 2009-08-18 13:17:47 PDT
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.
Comment 9 Yong Li 2009-08-18 13:23:13 PDT
(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?
Comment 10 Yael 2009-12-12 07:44:28 PST
Created attachment 44738 [details]
Patch. 

Implement Antti's proposal to break the script execution in HTMLTokenizer.cpp.
Comment 11 WebKit Review Bot 2009-12-12 07:49:08 PST
style-queue ran check-webkit-style on attachment 44738 [details] without any errors.
Comment 12 Antti Koivisto 2009-12-14 01:31:05 PST
Comment on attachment 44738 [details]
Patch. 

Looks good, r=me
Comment 13 WebKit Commit Bot 2009-12-14 07:21:49 PST
Comment on attachment 44738 [details]
Patch. 

Clearing flags on attachment: 44738

Committed r52091: <http://trac.webkit.org/changeset/52091>
Comment 14 WebKit Commit Bot 2009-12-14 07:21:55 PST
All reviewed patches have been landed.  Closing bug.