Open Bug 1286026 Opened 3 years ago Updated 5 months ago

2,100 instances of "NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed" emitted from dom/base/nsScriptLoader.cpp during linux64 debug testing

Categories

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

defect

Tracking

()

People

(Reporter: erahm, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

> 1875 WARNING: NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed: file dom/base/nsScriptLoader.cpp, line 1287

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

>    741 - [TC] Linux64 mochitest-gl gl
>    741 - [TC] Linux64 mochitest-gl-e10s gl
>     66 - [TC] Linux64 web-platform-tests-6 6
>     66 - [TC] Linux64 web-platform-tests-e10s-6 6
>     33 - [TC] Linux64 web-platform-tests-5 5
>     33 - [TC] Linux64 web-platform-tests-e10s-5 5
>     22 - [TC] Linux64 mochitest-9 9
>     22 - [TC] Linux64 mochitest-plain-e10s-9 9
>     16 - [TC] Linux64 web-platform-tests-e10s-7 7
>     16 - [TC] Linux64 web-platform-tests-7 7
>     15 - [TC] Linux64 web-platform-tests-4 4
>     15 - [TC] Linux64 web-platform-tests-e10s-4 4
>      9 - [TC] Linux64 mochitest-gpu-e10s gpu
>      9 - [TC] Linux64 mochitest-gpu gpu
>      8 - [TC] Linux64 firefox-ui-tests functional remote e10s en-US
>      8 - [TC] Linux64 firefox-ui-tests functional remote en-US
>      6 - [TC] Linux64 mochitest-plain-e10s-2 2
>      5 - [TC] Linux64 web-platform-tests-e10s-10 10
>      5 - [TC] Linux64 web-platform-tests-10 10
>      4 - [TC] Linux64 mochitest-2 2
>      4 - [TC] Linux64 mochitest-3 3
>      3 - [TC] Linux64 mochitest-plain-e10s-4 4
>      3 - [TC] Linux64 web-platform-tests-e10s-3 3
>      3 - [TC] Linux64 web-platform-tests-e10s-2 2
>      3 - [TC] Linux64 web-platform-tests-2 2
>      3 - [TC] Linux64 web-platform-tests-3 3
>      2 - [TC] Linux64 mochitest-media mda
>      2 - [TC] Linux64 mochitest-media-e10s mda
>      2 - [TC] Linux64 mochitest-4 4
>      2 - [TC] Linux64 mochitest-plain-e10s-3 3
>      1 - [TC] Linux64 web-platform-tests-9 9
>      1 - [TC] Linux64 web-platform-tests-e10s-9 9
>      1 - [TC] Linux64 web-platform-tests-e10s-11 11
>      1 - [TC] Linux64 mochitest-5 5
>      1 - [TC] Linux64 firefox-ui-tests functional local e10s en-US
>      1 - [TC] Linux64 web-platform-tests-e10s-12 12
>      1 - [TC] Linux64 web-platform-tests-12 12
>      1 - [TC] Linux64 web-platform-tests-11 11

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

>     50 -        dom/canvas/test/webgl-conf/generated/test_conformance__context__context-release-upon-reload.html
>     50 -        dom/canvas/test/webgl-conf/generated/test_conformance__context__context-release-with-workers.html
>     50 - [e10s] dom/canvas/test/webgl-conf/generated/test_conformance__context__context-release-with-workers.html
>     50 - [e10s] dom/canvas/test/webgl-conf/generated/test_conformance__context__context-release-upon-reload.html
>     40 - [e10s] /html/syntax/parsing/html5lib_domjs-unsafe.html?run_type=NNNNNN
>     40 -        /html/syntax/parsing/html5lib_domjs-unsafe.html?run_type=NNNNNN
>     33 -        /html/semantics/scripting-1/the-script-element/script-languages-02.html
>     33 - [e10s] /html/semantics/scripting-1/the-script-element/script-languages-02.html
>     22 -        parser/htmlparser/tests/mochitest/test_html5_tree_construction.html
>     22 - [e10s] parser/htmlparser/tests/mochitest/test_html5_tree_construction.html

[1] https://hg.mozilla.org/mozilla-central/annotate/214884d507ee/dom/base/nsScriptLoader.cpp#l1287
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
You need to log in before you can comment on or make changes to this bug.