Closed Bug 1286026 Opened 8 years ago Closed 4 months ago

12,200 instances of WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type) || type.LowerCaseEqualsASCII("module") || type.LowerCaseEqualsASCII("importmap")) failed from ScriptElement.cpp during linux64 debug testing

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
128 Branch
Tracking Flags:
Tracking Status
firefox128 --- fixed

People

(Reporter: erahm, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1286026 - Don't warn for invalid WebGL script types, and include the invalid script type.
48 bytes, text/x-phabricator-request
Details | Review
Not sure what you've bisected; neither of those commits seem likely to cause this warning to appear. In either case, this is a very useful warning on real sites. It is perhaps less useful for test cases that explicitly test this exact line of code (like script-languages-02.html), or pages like test_conformance__context__context-release-upon-reload.html that reload themselves 25 times.
(In reply to Eric Rahm [:erahm] from comment #1) Indedd, and I confirmed that I see this warning at the revision before those changes.
Flags: needinfo?(jcoppeard)
Sorry for the confusion, apparently it bisected that your patch moved the line number in the warning.
Looks like this warning spam has been around since before I can bisect (2016-03-15 or so), not related to 1281194.
No longer blocks: 1281194
Priority: -- → P3
This just converts the NS_ENSURE_TRUE to an if statement. Jon, you were the last one to poke this code so I chose you, but feel free to redirect.
Attachment #8774926 - Flags: review?(jcoppeard)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8774926 [details] [diff] [review] Remove ParseTypeAttribute warning This isn't really my area so I'm going to punt it to someone who sounds like they know more about it.
Attachment #8774926 - Flags: review?(jcoppeard) → review?(Ms2ger)
Comment on attachment 8774926 [details] [diff] [review] Remove ParseTypeAttribute warning Review of attachment 8774926 [details] [diff] [review]: ----------------------------------------------------------------- Since you ask me... I don't think this is an improvement. You could try to find someone to override me, of course.
Attachment #8774926 - Flags: review?(Ms2ger) → review-
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #8) > Comment on attachment 8774926 [details] [diff] [review] > Remove ParseTypeAttribute warning > > Review of attachment 8774926 [details] [diff] [review]: > ----------------------------------------------------------------- > > Since you ask me... I don't think this is an improvement. You could try to > find someone to override me, of course. Do you find 1875 warnings useful? Are they actionable? Or was NS_ENSURE_TRUE used as a macro-fu shorthand to avoid typing out a full |if| statement? If it's actionable are you interested in fixing the errors? If they're errors should we make this an assert?
Flags: needinfo?(Ms2ger)
The usefulness comes when debugging Gecko's behavior on some websites. NS_ENSURE_* macros in general are super useful for that - often immediately showing where things start to go wrong. (I don't personally care too much whether we keep this particular warning)
(In reply to Olli Pettay [:smaug] from comment #10) > The usefulness comes when debugging Gecko's behavior on some websites. > NS_ENSURE_* macros in general are super useful for that - often immediately > showing where things start to go wrong. > > > (I don't personally care too much whether we keep this particular warning) Would a |MOZ_LOG| be more appropriate?
MOZ_LOG is usually overkill.
This continues to be a top warning and has become slightly more verbose.
Summary: 1,900 instances of "NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed" emitted from dom/base/nsScriptLoader.cpp during linux64 debug testing → 2,000 instances of "NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed" emitted from dom/base/nsScriptLoader.cpp during linux64 debug testing
This continues to be a top warning and has become slightly more verbose.
Summary: 2,000 instances of "NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed" emitted from dom/base/nsScriptLoader.cpp during linux64 debug testing → 2,100 instances of "NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed" emitted from dom/base/nsScriptLoader.cpp during linux64 debug testing
Flags: needinfo?(Ms2ger)
Assignee: erahm → nobody
Status: ASSIGNED → NEW
ParseTypeAttribute was removed, but ProcessScriptElement still warns if nsContentUtils::IsJavascriptMIMEType returns false. I don't think it mess sense to spam whenever *web pages* have an invalid type attribute in <script> elements.
Component: DOM → DOM: Core & HTML
Severity: normal → S3

It looks like the warning related to nsContentUtils::IsJavascriptMIMEType() has migrated again, to ScriptElement::MaybeProcessScript().

12,200 instances of "WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type) || type.LowerCaseEqualsASCII("module") || type.LowerCaseEqualsASCII("importmap")) failed: file dom/script/ScriptElement.cpp:163" emitted from none during linux1804-64-qr debug testing

12232 WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type) || type.LowerCaseEqualsASCII("module") || type.LowerCaseEqualsASCII("importmap")) failed: file dom/script/ScriptElement.cpp:163

This warning [1] shows up in the following test suites:

   542 - test-linux1804-64-qr/debug-mochitest-webgl1-ext-nofis gl1e
   542 - test-linux1804-64-qr/debug-mochitest-webgl1-ext-swr gl1e
   542 - test-linux1804-64-qr/debug-mochitest-webgl1-ext-swr-nofis gl1e
   542 - test-linux1804-64-qr/debug-mochitest-webgl1-ext-gli gl1e
   542 - test-linux1804-64-qr/debug-mochitest-webgl1-ext gl1e
   510 - test-linux1804-64-qr/debug-mochitest-webgl2-core-swr gl2c
   510 - test-linux1804-64-qr/debug-mochitest-webgl2-core-nofis gl2c
   510 - test-linux1804-64-qr/debug-mochitest-webgl2-core-gli gl2c
   510 - test-linux1804-64-qr/debug-mochitest-webgl2-core-swr-nofis gl2c
   510 - test-linux1804-64-qr/debug-mochitest-webgl2-core gl2c
   408 - test-linux1804-64-qr/debug-mochitest-webgl1-core gl1c
   408 - test-linux1804-64-qr/debug-mochitest-webgl1-core-nofis gl1c
   408 - test-linux1804-64-qr/debug-mochitest-webgl1-core-gli gl1c
   408 - test-linux1804-64-qr/debug-mochitest-webgl1-core-swr gl1c
   408 - test-linux1804-64-qr/debug-mochitest-webgl1-core-swr-nofis gl1c
   [...]

It shows up in 950 tests. A few of the most prevalent:

   250 -        dom/canvas/test/webgl-conf/generated/test_2_conformance__context__context-release-upon-reload.html
   250 -        dom/canvas/test/webgl-conf/generated/test_2_conformance__context__context-release-with-workers.html
   250 -        dom/canvas/test/webgl-conf/generated/test_conformance__context__context-release-upon-reload.html
   250 -        dom/canvas/test/webgl-conf/generated/test_conformance__context__context-release-with-workers.html
   154 -        parser/htmlparser/tests/mochitest/test_html5_tree_construction.html
   130 -        /html/semantics/scripting-1/the-script-element/script-type-and-language-js-svg.svg
   130 -        /html/semantics/scripting-1/the-script-element/script-type-and-language-js-xhtml.xhtml
   130 -        /html/semantics/scripting-1/the-script-element/script-type-and-language-js.html
   110 -        dom/canvas/test/webgl-conf/generated/test_2_conformance2__glsl3__bool-type-cast-bug-uint-ivec-uvec.html
   100 -        dom/canvas/test/webgl-conf/generated/test_2_conformance__glsl__bugs__bool-type-cast-bug-int-float.html

[1] https://hg.mozilla.org/mozilla-central/annotate/99a425a15992bd6c756462854861e61c6a1785f2/none#l0

Summary: 2,100 instances of "NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed" emitted from dom/base/nsScriptLoader.cpp during linux64 debug testing → 12,200 instances of WARNING: NS_ENSURE_TRUE(nsContentUtils::IsJavascriptMIMEType(type) || type.LowerCaseEqualsASCII("module") || type.LowerCaseEqualsASCII("importmap")) failed from ScriptElement.cpp during linux64 debug testing

Given that the overwhelming majority of these errors are in the WebGL1 and WebGL2 test suites, my guess is that the test suite is doing something incorrectly that is causing these errors to happen.

emk's point in comment 25 that it doesn't make sense to warn when a web page is missing a tag (I guess that's what the warning is for?) sounds reasonable to me. Are we expecting that web developers are browsing using debug builds? Surely not.

I wrote a little patch to improve this error message, so it actually includes the type. I'll file a separate bug for that. Anyways, with that patch I ran the test_2_conformance__context__context-release-upon-reload.html test. It looks like the unknown script types are "x-shader/x-vertex" and "x-shader/x-fragment". According to an answer I found on Stack Overflow, it sounds like it is a WebGL convention to stuff random strings they need into script tags using these deliberately-invalid values. Given the convention, it might be worth skipping the warning for these specific values. I dunno how many variants there might be.

Assignee: nobody → continuation

This patch changes the invalid script tag warning to actually include the
invalid tag, which makes it easier to understand what is going wrong.

WebGL pages use invalid types on script elements to store strings. These three
specific strings are used in hundreds of places in our WebGL tests, so don't
warn for them. The first two can be found in WebGL tutorials, so this seems like
an actual convention. "text/something-not-javascript" looks like it is only
used in the WebGL conformance tests, but it is clearly deliberately invalid
so skip warning for it, too, in the interests of reducing warning spam.

I didn't use NS_WARNING_ASSERTION here because that includes the entire
expression that failed, and that is large and not very useful.

Attachment #8774926 - Attachment is obsolete: true

I think my patch will eliminate at least 90% of these warnings, and for any remainders, the message will change to include the actual invalid script tag, so this bug can be closed when my patch lands. If there are any problematic individual tags remaining, we can file a new bug.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2d6b6f37c364 Don't warn for invalid WebGL script types, and include the invalid script type. r=dom-core,farre

https://hg.mozilla.org/mozilla-central/rev/2d6b6f37c364

Status: NEW → RESOLVED
Closed: 4 months ago
status-firefox128: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 128 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: