Bug 51760 – Changeset #67245 was incompletely implemented

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kyle Simpson]

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.

[Kyle Simpson]

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.

[Ryosuke Niwa]

Eric & Tony: is this bug valid?

[Adam Barth]

We've been discussing the issue in the HTML working group.  The issue was resolved there, but I don't remember the outcome.

[Kyle Simpson]

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).

[Tony Gentilcore]

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/[e-mail]/msg25171.html. Kyle makes a very good case in http://www.mail-archive.com/[e-mail]/msg25174.html.

[James Simonsen]

Created attachment 81741 [details]
Patch

[Adam Barth]

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.

[James Simonsen]

Created attachment 81746 [details]
Patch

[James Simonsen]

(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.

[Tony Gentilcore]

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.

[Adam Barth]

> 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.

[James Simonsen]

Created attachment 81859 [details]
Patch

[James Simonsen]

(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.

[Tony Gentilcore]

Comment on attachment 81859 [details]
Patch

LGTM

[Kyle Simpson]

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?

[Tony Gentilcore]

(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.

[James Simonsen]

Created attachment 84787 [details]
Patch

[James Simonsen]

(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.

[WebKit Commit Bot]

Comment on attachment 84787 [details]
Patch

Clearing flags on attachment: 84787

Committed r80406: <http://trac.webkit.org/changeset/80406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.