Closed Bug 841802 Opened 12 years ago Closed 12 years ago

"Assertion failure: rv != nsresult::NS_ERROR_ILLEGAL_INPUT" with UTF-16 document and fragment identifier

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: jruderman, Assigned: emk)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 3 obsolete files)

stack
(deleted), text/plain
Details
Part 1: Add ErrorResult::ClearMessage()
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 2: Use TextDecoderBase from nsContentUtils::ConvertStringFromCharset()
(deleted), patch
hsivonen
: review+
Details | Diff | Splinter Review
Attached file stack (deleted) —
Loading file:////.../layout/reftests/css-charset/test-charset-utf-16-be-bom.css#a Assertion failure: rv != nsresult::NS_ERROR_ILLEGAL_INPUT, at content/base/src/nsContentUtils.cpp:3656
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #714588 - Flags: review?(bzbarsky)
Attachment #714590 - Flags: review?(hsivonen)
Blocks: 819868
Comment on attachment 714588 [details] [diff] [review] Part 1: Clear error message when ErrorResult::ErrorCode() is called This seems like very unexpected behavior to me.... And in particular, aren't we catching a real bug right now where the exception is being dropped on the floor?
And in particular, if part 1 is only needed to make part 2 work, then I think we should have an explicit way of dropping the non-nsresult parts of our state. If desired that way can even return the nsresult.
Forgot that mMessage might not be initialized. https://tbpl.mozilla.org/?tree=Try&rev=c356d6973b53
Attachment #714588 - Attachment is obsolete: true
Attachment #714588 - Flags: review?(bzbarsky)
Attachment #714741 - Flags: review?(bzbarsky)
Comment on attachment 714741 [details] [diff] [review] Part 1: Clear error message when ErrorResult::ErrorCode() is called Oh, mid-aired.
Attachment #714741 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #4) > And in particular, if part 1 is only needed to make part 2 work, then I > think we should have an explicit way of dropping the non-nsresult parts of > our state. If desired that way can even return the nsresult. OK, made ClearMessage() explicit.
Attachment #714741 - Attachment is obsolete: true
Attachment #714745 - Flags: review?(bzbarsky)
Using ClearMessage() explicitly.
Attachment #714590 - Attachment is obsolete: true
Attachment #714590 - Flags: review?(hsivonen)
Attachment #714746 - Flags: review?(hsivonen)
Comment on attachment 714745 [details] [diff] [review] Part 1: Add ErrorResult::ClearMessage() r=me
Attachment #714745 - Flags: review?(bzbarsky) → review+
Comment on attachment 714746 [details] [diff] [review] Part 2: Use TextDecoderBase from nsContentUtils::ConvertStringFromCharset() Sorry. I'm not up to speed on how the new bindings work. Why is it necessary to clear the message before the first return site but not before the second return site?
(In reply to Henri Sivonen (:hsivonen) from comment #10) > Comment on attachment 714746 [details] [diff] [review] > Part 2: Use TextDecoderBase from nsContentUtils::ConvertStringFromCharset() > > Sorry. I'm not up to speed on how the new bindings work. Why is it necessary > to clear the message before the first return site but not before the second > return site? Because only the first return site can throw TypeError. Basically TypeError should be thrown from generated codes as much as possible. TextDecoder's constructor is an exceptional case.
Comment on attachment 714746 [details] [diff] [review] Part 2: Use TextDecoderBase from nsContentUtils::ConvertStringFromCharset() r+ on the assumption that ClearMessage() usage makes sense.
Attachment #714746 - Flags: review?(hsivonen) → review+
> Comment #0 implies that our existing test case already covers this. That's not obvious to me, since the tree was green before this fix...
Maybe it's in bug 594645? (I have no access.)
That bug is a bug with a fuzzer. As in, this bug was caught during fuzz-testing, using one of our reftests as the fuzzer input. Which means we need a non-fuzzer test for this.
Comment 0 does not imply that our existing test suite covers this. Our existing test suite does not involve loading test-charset-utf-16-be-bom.css with a fragment identifier (or alone, at all).
Flags: in-testsuite+ → in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/a093423c5e5b https://hg.mozilla.org/mozilla-central/rev/3b15e153f662
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: