1301251 - Handle GetSpec() failure in nsContentUtils::GetWrapperSafeScriptFilename()

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

We need to better handle failure of GetSpec() in nsContentUtils::GetWrapperSafeScriptFilename(). In bug 1297300 bz suggests doing so by:

> Arguably by not executing the relevant script.  Or by removing this function
> altogether; it's not clear to me whether it does anything useful nowadays...

If we do keep the function, it might be worth splitting it into two parts. Because there are two output values -- the return value (bool), and the output string, and each callsite only uses one of the outputs.

Comment 1

[Nicholas Nethercote [inactive]]

And note that there are two GetSpec() calls in nsContentUtils::GetWrapperSafeScriptFilename().

Comment 2

[Andrew Overholt [:overholt]]

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

Comment 3

[Andrea Marchesini [:baku]]

Attachment: doc3.patch

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8791143 [details] [diff] [review]
doc3.patch

Review of attachment 8791143 [details] [diff] [review]:
-----------------------------------------------------------------

This looks plausible, but I'm not a DOM peer and not comfortable giving r+. I was going to ask bz to review, but he's not accepting reviews at the moment.

::: dom/base/nsScriptLoader.h
@@ +548,5 @@
>                             nsScriptLoadRequest* aRequest);
>    nsresult EvaluateScript(nsScriptLoadRequest* aRequest);
>  
>    already_AddRefed<nsIScriptGlobalObject> GetScriptGlobalObject();
> +  nsresult FillCompileOptionsForRequest(const mozilla::dom::AutoJSAPI& jsapi,

Make this MOZ_MUST_USE.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8791143 [details] [diff] [review]
doc3.patch

I don't see reason to use ErrorResult here. nsresult would be stronger hint that we're dealing with gecko internal stuff.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: doc3.patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8791636 [details] [diff] [review]
doc3.patch

>+nsContentUtils::GetWrapperSafeScriptFilename(nsIDocument* aDocument,
>+                                             nsIURI* aURI,
>+                                             nsACString& aScriptURI,
>+                                             nsresult* aRv)
>+{
>+  MOZ_ASSERT(aRv);
>   bool scriptFileNameModified = false;
>-  // XXX: should handle GetSpec() failure properly. See bug 1301251.
>-  Unused << aURI->GetSpec(aScriptURI);
>+  *aRv = NS_OK;
>+
>+  *aRv = aURI->GetSpec(aScriptURI);
>+  if (NS_WARN_IF(NS_FAILED(*aRv))) {
>+    return false;
>+  }
I would probably use
NS_ENSURE_SUCCESS(*aRv, false)
here and elsewhere but up to you


Make sure to push this to tryserver since the changes to XBL and XUL might cause some unexpected test failures.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/553eb2fc3a71
Handle GetSpec() failure in nsContentUtils::GetWrapperSafeScriptFilename(), r=smaug

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/553eb2fc3a71

Comment 10

[Gerry Chang [:gchang]]

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