This is a spin-off of bug 1297300, which is adding checking for GetSpec() calls that are lacking them. nsIDocument::GetDocumentURI() has a GetSpec() call that is fallible. The right way to handle this is to make nsIDocument::GetDocumentURI() itself fallible. bz noted in bug 1297300 comment 31: > Make this funcation fallible. I just looked through the callers and some of > them want to throw exceptions to JS if this fails anyway, some shouldn't be > using this function at all because it causes extra encoding conversions > (NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(DOMEventTargetHelper), > restyle logging, style context asserts (via GetURL), global window code > checking for "about:neterror" (via GetURL) I'm looking at you), some could > start throwing exceptions to JS even though they don't right now.

And its return value should be MOZ_MUST_USE, of course :)

Please let me know if this should be a higher priority than "backlog".

> Please let me know if this should be a higher priority than "backlog". That seems fine.

>+ rv.SuppressException(); Can just use an IgnoredErrorResult.

>+ result.Append(NS_ConvertUTF16toUTF8(url).get()); I know this is preexisting, but... CopyUTF16toUTF8(url, result);

Comment on attachment 8791131 [details] [diff] [review] doc1.patch Review of attachment 8791131 [details] [diff] [review]: ----------------------------------------------------------------- I will defer to a DOM peer for r+.

Comment on attachment 8791131 [details] [diff] [review] doc1.patch I don't see reason for using ErrorResult when GetURL and GetDocumentURI could just return nsresult (possibly with MOZ_MUST_USE). IMO ErrorResults brings just too complicated setup for this kinds of Gecko internal methods.

Comment on attachment 8792486 [details] [diff] [review] doc1.patch > #define STYLE_STRUCT_INHERITED(name_, checkdata_cb) \ > data = destroyingInheritedData.mStyleStructs[eStyleStruct_##name_]; \ > if (data && \ > !(aDestroyingContext->mBits & NS_STYLE_INHERIT_BIT(name_)) && \ > (mCachedInheritedData.mStyleStructs[eStyleStruct_##name_] == data)) { \ > printf_stderr("style struct %p found on style context %p\n", data, this);\ > nsString url; \ >- PresContext()->Document()->GetURL(url); \ >- printf_stderr(" in %s\n", NS_ConvertUTF16toUTF8(url).get()); \ >- MOZ_ASSERT(false, "destroying " #name_ " style struct still present " \ >- "in style context tree"); \ >+ nsresult rv = PresContext()->Document()->GetURL(url); \ >+ if (NS_SUCCEEDED(rv)) { \ >+ printf_stderr(" in %s\n", NS_ConvertUTF16toUTF8(url).get()); \ >+ MOZ_ASSERT(false, "destroying " #name_ " style struct still present " \ >+ "in style context tree"); \ >+ } \ align \ characters and I would indent stuff inside the 'if' to keep this stuff readable. You don't want MOZ_ASSERT to be inside the 'if'. > #define STYLE_STRUCT_RESET(name_, checkdata_cb) \ > data = destroyingResetData->mStyleStructs[eStyleStruct_##name_]; \ > if (data && \ > !(aDestroyingContext->mBits & NS_STYLE_INHERIT_BIT(name_)) && \ > (mCachedResetData->mStyleStructs[eStyleStruct_##name_] == data)) { \ > printf_stderr("style struct %p found on style context %p\n", data, \ > this); \ > nsString url; \ >- PresContext()->Document()->GetURL(url); \ >- printf_stderr(" in %s\n", NS_ConvertUTF16toUTF8(url).get()); \ >- MOZ_ASSERT(false, "destroying " #name_ " style struct still present "\ >- "in style context tree"); \ >+ nsresult rv = PresContext()->Document()->GetURL(url); \ >+ if (NS_SUCCEEDED(rv)) { \ >+ printf_stderr(" in %s\n", NS_ConvertUTF16toUTF8(url).get()); \ >+ MOZ_ASSERT(false, "destroying " #name_ " style struct still present "\ >+ "in style context tree"); \ >+ } \ MOZ_ASSERT shouldn't be inside the 'if', And align \ Or other option is to explicitly say that return value of GetURL isn't used. Those fixed, r+

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa252c2f85b5 nsIDocument::GetDocumentURI() should be fallible, r=smaug

Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.