Closed
Bug 927262
Opened 11 years ago
Closed 11 years ago
MOZ_ASSERT(NS_SUCCEEDED(rv)) failed in mozilla::dom::Element::GetMarkup
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: jruderman, Assigned: smaug)
References
Details
(Keywords: assertion, regression, testcase)
Attachments
(2 files)
319 bytes,
text/html
|
Details | |
1.88 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 1•11 years ago
|
||
haa, this revealed a real bug.
Attachment #817785 -
Flags: review?(hsivonen)
Comment 2•11 years ago
|
||
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?
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
review ping
Assignee | ||
Comment 5•11 years ago
|
||
(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 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/42b04948eedd
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd45c8ba5ff under suspicion of breaking mochitest-metro tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=29509153&tree=Mozilla-Inbound
mbrubeck says it's more likely bug 914847 that caused the bustage, so I backed that out separately from this bug.
Comment 10•11 years ago
|
||
Re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a22cfe165fa
Status: NEW → ASSIGNED
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6a22cfe165fa
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•