Bug 51760 - Changeset #67245 was incompletely implemented
: Changeset #67245 was incompletely implemented
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
http://test.getify.com/webkit-bug-onl...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-30 12:13 PST by Kyle Simpson
Modified: 2011-03-04 21:41 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.34 KB, patch)
2011-02-08 21:19 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (3.43 KB, patch)
2011-02-08 21:49 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (4.46 KB, patch)
2011-02-09 13:18 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (2.60 KB, patch)
2011-03-04 12:02 PST, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle Simpson 2010-12-30 12:13:19 PST
Webkit back in September landed changeset 67245, which effectively tells the browser not to fetch a script resource if the `type` attribute is invalid/unrecognized and the browser doesn't intend to execute it.

http://www.w3.org/TR/html5/scripting-1.html#running-a-script

Here's the wording in the spec, in steps 5 and 6: 

[[ 
If the user agent does not support the scripting language given by the script block's type for this script element, then the user agent must abort these steps at this point.
]] 

It would seem pretty likely that this change should apply to both parser-inserted (markup) script elements as well as script-inserted (dynamic) script elements. All other parts of the spec wording in this part of the algorithm are indeed applied to both types of script elements, so this should be, too.

However, if you inspect the provided test page in a recent Webkit nightly, you'll see that the changeset above only affected markup script elements (the "jquery.min.js" was not fetched). The dynamic script element for "dojo.xd.js" should not have been fetched, and yet it was.
Comment 1 Kyle Simpson 2010-12-30 12:57:33 PST
Someone removed my assertion of a relationship to bug #50115. I am asserting that if THIS bug is implemented, it will directly affect (and give even more importance to) bug #50115.
Comment 2 Ryosuke Niwa 2011-02-01 13:38:35 PST
Eric & Tony: is this bug valid?
Comment 3 Adam Barth 2011-02-01 14:19:02 PST
We've been discussing the issue in the HTML working group.  The issue was resolved there, but I don't remember the outcome.
Comment 4 Kyle Simpson 2011-02-01 14:51:12 PST
The reason I filed this bug is because some Webkit developers were (in bug #50115) working on implementing my "async=false" proposal, and there were questions as to why it was even necessary for Webkit. Upon investigating those questions, I discovered, contradicting my previous assumptions, that changeset #67245 was only applied to parser-inserted scripts, which is why the breakage I expected to see in Webkit nightlies was in fact not happening. 

I brought this to those devs' attention, asking if the changeset was intended for only parser-inserted scripts, and in comment-49 of bug #50115, Adam said: 

"It's not intentional. Please file a new bug (and ideally attach a patch)."

...which is why *this* bug was filed. His assertion is that changeset #67245 was *intended* (as clearly implied from the spec wording from which it came) to apply to both parser-inserted AND script-inserted script elements.

The whole "reason" for needing the "async=false" proposal from bug #50115 implemented in Webkit was based on my assumption that changeset #67245 was fully implemented for both types of script elements, which would have lead to breakage in script loaders.

You may be tempted to say "well, that's easy then, just leave things as-is and don't implement the breaking change." But as I said, I agree with Adam and I think the spec wording clearly indicates that the change *should* apply to both types of script elements. In my opinion, the proper thing is to do both this bug (which will break script loaders) and then bug #50115 (which will fix them again).
Comment 5 Tony Gentilcore 2011-02-05 16:49:22 PST
Hixie has a nice summary here: http://ln.hixie.ch/?start=1296711456&count=1

I pretty much agree with everything he says and think we should do both this and bug 50115 (which is now specced). We just have to make sure to land them as close as possible together (if not as a part of the same patch).

Another pro of async=false is that it might eliminate the need for the noexecute attribute proposed in http://www.mail-archive.com/whatwg@lists.whatwg.org/msg25171.html. Kyle makes a very good case in http://www.mail-archive.com/whatwg@lists.whatwg.org/msg25174.html.
Comment 6 James Simonsen 2011-02-08 21:19:07 PST
Created attachment 81741 [details]
Patch
Comment 7 Adam Barth 2011-02-08 21:22:56 PST
Comment on attachment 81741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81741&action=review

Clever test!

> LayoutTests/fast/dom/HTMLScriptElement/dont-load-unknown-type.html:14
> +<!-- Unknown script types should not be loaded nor executed. If this test fails, the following text will appear in the output:

Can we make some of this text appear in the expected results?  Generally having empty expected.txt files is a pain.
Comment 8 James Simonsen 2011-02-08 21:49:01 PST
Created attachment 81746 [details]
Patch
Comment 9 James Simonsen 2011-02-08 21:51:28 PST
(In reply to comment #7)
> (From update of attachment 81741 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81741&action=review
> 
> Clever test!
> 
> > LayoutTests/fast/dom/HTMLScriptElement/dont-load-unknown-type.html:14
> > +<!-- Unknown script types should not be loaded nor executed. If this test fails, the following text will appear in the output:
> 
> Can we make some of this text appear in the expected results?  Generally having empty expected.txt files is a pain.

Done.

I will hold off on submitting this until bug 50115 is fixed.
Comment 10 Tony Gentilcore 2011-02-09 08:59:24 PST
Comment on attachment 81746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81746&action=review

> Source/WebCore/dom/ScriptElement.cpp:160
> +    if (!shouldExecuteAsJavaScript())

Since the beforeload event isn't standardized, the spec doesn't have anything to say about this. But as this is placed, we would still fire a beforeload event when we aren't actually planning to load the script.

My first thought is that this check should be performed before the beforeload is fired. But OTOH, the beforeload event could also change attributes which affect the result of this check, so I can see an argument for after beforeload as well. Maybe check how parser inserted scripts behave and do the same? In any case, I'd recommend adding a test since this behavior is subtle.

Everything else lgtm.
Comment 11 Adam Barth 2011-02-09 11:15:36 PST
> Since the beforeload event isn't standardized, the spec doesn't have anything to say about this. But as this is placed, we would still fire a beforeload event when we aren't actually planning to load the script.

Maybe we should do this check twice?  Once before the event and then again afterwards?  Definitely should be tested.
Comment 12 James Simonsen 2011-02-09 13:18:35 PST
Created attachment 81859 [details]
Patch
Comment 13 James Simonsen 2011-02-09 13:21:40 PST
(In reply to comment #11)
> > Since the beforeload event isn't standardized, the spec doesn't have anything to say about this. But as this is placed, we would still fire a beforeload event when we aren't actually planning to load the script.
> 
> Maybe we should do this check twice?  Once before the event and then again afterwards?  Definitely should be tested.

I implemented it this way and added test cases for all the combinations.

There's a FIXME in there to reduce some redundancy. The best way to address it would be to rewrite more of ScriptElement to match the HTML5 spec. I'll get to that later with a separate issue & patch.
Comment 14 Tony Gentilcore 2011-02-09 13:59:56 PST
Comment on attachment 81859 [details]
Patch

LGTM
Comment 15 Kyle Simpson 2011-03-04 05:28:53 PST
It would appear this change has landed, as Chrome nightlies are breaking LABjs and the symptom is exactly this functionality, which is not fetching of script elements with fake mime-types. Has it in fact landed? Did Chrome just pick it up early?

I was under the impression that *this* bug and Bug #50115 would be landed together to prevent breakage of LABjs. But 50115 is still in progress.

Can you advise?
Comment 16 Tony Gentilcore 2011-03-04 08:48:59 PST
(In reply to comment #15)
> It would appear this change has landed, as Chrome nightlies are breaking LABjs and the symptom is exactly this functionality, which is not fetching of script elements with fake mime-types. Has it in fact landed? Did Chrome just pick it up early?

http://trac.webkit.org/changeset/79114 lined us up w/ the spec better and probably implemented this. We should make sure the test in this patch still lands and move forward w/ bug 50115 asap to address the problem.
Comment 17 James Simonsen 2011-03-04 12:02:52 PST
Created attachment 84787 [details]
Patch
Comment 18 James Simonsen 2011-03-04 12:04:35 PST
(In reply to comment #16)
> (In reply to comment #15)
> > It would appear this change has landed, as Chrome nightlies are breaking LABjs and the symptom is exactly this functionality, which is not fetching of script elements with fake mime-types. Has it in fact landed? Did Chrome just pick it up early?
> 
> http://trac.webkit.org/changeset/79114 lined us up w/ the spec better and probably implemented this. We should make sure the test in this patch still lands and move forward w/ bug 50115 asap to address the problem.

Yeah, it looks like that's what happened. This layout test, with no other changes, now passes. It must've been 79114 that changed the behavior.

I've reverted the code changes from this patch and am now just going to submit the new layout test.
Comment 19 WebKit Commit Bot 2011-03-04 21:41:30 PST
Comment on attachment 84787 [details]
Patch

Clearing flags on attachment: 84787

Committed r80406: <http://trac.webkit.org/changeset/80406>
Comment 20 WebKit Commit Bot 2011-03-04 21:41:36 PST
All reviewed patches have been landed.  Closing bug.