Closed Bug 927262 Opened 6 years ago Closed 6 years ago

MOZ_ASSERT(NS_SUCCEEDED(rv)) failed in mozilla::dom::Element::GetMarkup

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files)

Attached file p.html
Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at content/base/src/Element.cpp:3167

The line was added to Element::GetMarkup in bug 923913:

  MOZ_ASSERT(NS_SUCCEEDED(rv));
Assignee: nobody → bugs
Attached patch patchSplinter Review
haa, this revealed a real bug.
Attachment #817785 - Flags: review?(hsivonen)
Could you please explain why it's wrong to cache the encoder in the  case that the patch makes it non-cached?

Also, before and after this patch, the code is okay with caching an encoder in the case where the node itself is not included in the serialization but it's not okay with caching the encoder when the node is not included in the serialization. Yet, the code is okay with using the cached encoder in both those cases and even continuing to keep a pre-cached encoder cached when it's used in a manner that would cause a new encoder not to be cached. How can that be right?
We create encoder and serializer based on the content type, and we default to application/xml.
Then if we cache encoder, yet document's real content type is something else, like in the
case of the testcase in this bug, we re-initialize the encoder to some dummy type which
can't be actually handled, and encoding starts to throw exceptions.

I'm not changing the aIncludeSelf handling. It should be probably just fine to cache in that case too,
but that is not about this bug.
review ping
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> Also, before and after this patch, the code is okay with caching an encoder
> in the case where the node itself is not included in the serialization but
> it's not okay with caching the encoder when the node is not included in the
> serialization. Yet, the code is okay with using the cached encoder in both
> those cases and even continuing to keep a pre-cached encoder cached when
> it's used in a manner that would cause a new encoder not to be cached. How
> can that be right?
And this is not about this bug.
Comment on attachment 817785 [details] [diff] [review]
patch

r+ according to the point that the weird part is out of scope for this bug.
Attachment #817785 - Flags: review?(hsivonen) → review+
mbrubeck says it's more likely bug 914847 that caused the bustage, so I backed that out separately from this bug.
https://hg.mozilla.org/mozilla-central/rev/6a22cfe165fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.