Closed
Bug 749367
Opened 12 years ago
Closed 12 years ago
<script type="text/template"> content parsed as JavaScript (+E4X)
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: potch, Assigned: capella)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
6.29 KB,
patch
|
jst
:
review+
Ms2ger
:
feedback+
|
Details | Diff | Splinter Review |
Observed in Nightly 15.0a1 (2012-04-26) It's become a common pattern [1] in web applications to perform JS templating by placing HTML snippets in a script tag with the type of "text/template". It is said (and assumed by developers) that content wrapped in such a way will not be parsed by browsers or displayed. I observed in the most recent Nightly that HTML inside these script tags is being evaluated as XML and triggering errors in the console. Here is a sample from https://marketplace-dev.allizom.org/en-US/ Timestamp: 4/26/12 1:55:01 PM Error: SyntaxError: XML can't be the whole program Source File: https://marketplace-dev.allizom.org/en-US/ Line: 337 This has the potential to cause issues with existing sites. [1]: https://github.com/documentcloud/backbone/blob/master/examples/todos/index.html
Reporter | ||
Updated•12 years ago
|
tracking-firefox15:
--- → ?
Comment 1•12 years ago
|
||
The problem is that scripts are typed text/template are parsed as JavaScript with E4X enabled. Since text/template isn't a JavaScript MIME type, the contents of those script elements shouldn't be passed to the JavaScript engine in the first place.
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
Updated•12 years ago
|
Summary: XML errors triggered when parsing <script type="text/template">content → <script type="text/template"> content parsed as JavaScript (+E4X)
Comment 2•12 years ago
|
||
By code inspection, this is a regression from bug 744332. The else branch at http://hg.mozilla.org/mozilla-central/annotate/807403a04a6a/content/base/src/nsScriptLoader.cpp#l470 is now bogus.
Blocks: 744332
Comment 3•12 years ago
|
||
This is a major web compat bug. It's common to use <script> tags like this for WebGL too, and it's actually possible that some WebGL shader bits can end up looking sorta like JS, I think. Mark, do you want to take this?
Severity: normal → major
Assignee | ||
Comment 4•12 years ago
|
||
yes... built the patch ... working on a mochitest
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•12 years ago
|
||
I may have let this fall out while ms2ger and I were working on separate / related bugs ... here's the rough patch ... Hoping a mochitest like this should suffice ... I've worked mostly in a11y so where would you say to add this test into? My first thought was to clone perhaps: tests/content/base/test/test_bug166235.html ... <script type="text/template"> is(true, false, "Shouldn't execute"); </script> <script type="text/javascript"> is(true, true, "Should execute"); </script>
Attachment #620879 -
Flags: feedback?(bzbarsky)
Updated•12 years ago
|
Keywords: regression
Assignee | ||
Updated•12 years ago
|
Attachment #620879 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 6•12 years ago
|
||
This patch builds and tests locally. The test fails before, works after. Running the whole test-suite ends suspiciously fast in my WIN7 environment, though qpop-ing and testing again ends fast also. Maybe something recent due to the PGO link problems ... ? Have a look, let me know -- mark
Attachment #620879 -
Attachment is obsolete: true
Attachment #620941 -
Flags: feedback?
Assignee | ||
Updated•12 years ago
|
Attachment #620941 -
Flags: feedback? → feedback?(Ms2ger)
Comment 7•12 years ago
|
||
Comment on attachment 620941 [details] [diff] [review] Patch (v2) Two things: 1) There are other places in the patch for bug 744332 that had similar issues. 2) I'd structure the test like this: <script> var ran = false; </script> <script type="text/template"> ran = true; </script> <script> is(ran, false, "text/template script should not run"); </script> and similar for any other types you want to test.
Comment 8•12 years ago
|
||
Comment on attachment 620941 [details] [diff] [review] Patch (v2) Review of attachment 620941 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! ::: content/base/src/nsScriptLoader.cpp @@ +465,5 @@ > } > } > > + if (!isJavaScript) > + typeID = nsIProgrammingLanguage::UNKNOWN; {}, please. ::: content/base/test/test_bug749367.html @@ +16,5 @@ > + <pre id="test"> > + > + <script type="text/template"> > + is(true, false, "Shouldn't execute"); > + is(true, true, "Shouldn't execute"); The canonical way to write this is 'ok(false...'. And the second check isn't necessary, right? @@ +20,5 @@ > + is(true, true, "Shouldn't execute"); > + </script> > + > + <script type="text/javascript"> > + is(true, true, "Should execute"); ok(true
Attachment #620941 -
Flags: feedback?(Ms2ger) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
Asking for feedback again ... found another block of identical code in nsXULContentSink.cpp that has now been addressed and a test created for. Again all builds and individually tests ok locally. The whole test-suite runs ok but seems to finish in about half the time it normally takes, though again, its doing this when I pop my change out also. Let me know if I should push to try, or go for the review? from JST ...
Attachment #620941 -
Attachment is obsolete: true
Attachment #621003 -
Flags: feedback?(Ms2ger)
Comment 10•12 years ago
|
||
Comment on attachment 621003 [details] [diff] [review] Patch (v3) Review of attachment 621003 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.
Attachment #621003 -
Flags: review?(jst)
Attachment #621003 -
Flags: feedback?(Ms2ger)
Attachment #621003 -
Flags: feedback+
Updated•12 years ago
|
Whiteboard: [autoland-try]
Updated•12 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Assignee | ||
Comment 11•12 years ago
|
||
ms2ger: pushed manually https://tbpl.mozilla.org/?tree=Try&rev=98bd00bb353a
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [autoland-in-queue]
Updated•12 years ago
|
status-firefox15:
--- → affected
Updated•12 years ago
|
Attachment #621003 -
Flags: review?(jst) → review+
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4522615e0fc3
Target Milestone: --- → mozilla15
Comment 13•12 years ago
|
||
Matt, how is that blocking rinmark? You shouldn't block ringmark but a ring number.
Comment 14•12 years ago
|
||
This bug does not affect any of the ringmark tests, but it causes errors in the rng.io test-runner HTML page. I had thought the errors were causing the test runner to fail, but it looks like they are non-fatal, so this probably doesn't need to block.
No longer blocks: ringmark
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4522615e0fc3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 16•12 years ago
|
||
Autoland Patchset: Patches: 621003 Branch: mozilla-central => try Patch 621003 could not be applied to mozilla-central. patching file content/base/src/nsScriptLoader.cpp Hunk #1 FAILED at 459 1 out of 1 hunks FAILED -- saving rejects to file content/base/src/nsScriptLoader.cpp.rej patching file content/base/test/Makefile.in Hunk #1 FAILED at 572 1 out of 1 hunks FAILED -- saving rejects to file content/base/test/Makefile.in.rej file content/base/test/test_bug749367.html already exists 1 out of 1 hunks FAILED -- saving rejects to file content/base/test/test_bug749367.html.rej patching file content/xul/content/test/Makefile.in Hunk #1 FAILED at 40 1 out of 1 hunks FAILED -- saving rejects to file content/xul/content/test/Makefile.in.rej file content/xul/content/test/test_bug749367.xul already exists 1 out of 1 hunks FAILED -- saving rejects to file content/xul/content/test/test_bug749367.xul.rej patching file content/xul/document/src/nsXULContentSink.cpp Hunk #1 FAILED at 922 1 out of 1 hunks FAILED -- saving rejects to file content/xul/document/src/nsXULContentSink.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir Patchset could not be applied and pushed.
Comment 19•12 years ago
|
||
Since web apps can only be installed on Nightly builds, I tried to verify this issue on Firefox 15.0 Nightly builds: 05/10 Nightly - the issue still reproduces (the push was only made that day, so this is ok) 05/11 Nightly & 05/27 Nightly - the Install button from the web app pages is unresponsive (nothing happens when clicking it & nothing gets displayed in the Error Console).
Comment 20•12 years ago
|
||
Verified as fixed on the 08/06 Firefox 17.0 Nightly.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•