Closed Bug 1301251 Opened 4 years ago Closed 4 years ago

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


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




Tracking Status
firefox51 --- fix-optional
firefox52 --- fixed


(Reporter: njn, Assigned: baku)



(1 file, 1 obsolete file)

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.
And note that there are two GetSpec() calls in nsContentUtils::GetWrapperSafeScriptFilename().
Please let me know if this should be a higher priority than "backlog".
Priority: -- → P3
Attached patch doc3.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8791143 - Flags: review?(n.nethercote)
Comment on attachment 8791143 [details] [diff] [review]

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.
Attachment #8791143 - Flags: review?(n.nethercote) → feedback+
Attachment #8791143 - Flags: review?(bugs)
Comment on attachment 8791143 [details] [diff] [review]

I don't see reason to use ErrorResult here. nsresult would be stronger hint that we're dealing with gecko internal stuff.
Attachment #8791143 - Flags: review?(bugs) → review-
Attached patch doc3.patchSplinter Review
Attachment #8791143 - Attachment is obsolete: true
Attachment #8791636 - Flags: review?(bugs)
Comment on attachment 8791636 [details] [diff] [review]

>+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
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.
Attachment #8791636 - Flags: review?(bugs) → review+
Pushed by
Handle GetSpec() failure in nsContentUtils::GetWrapperSafeScriptFilename(), r=smaug
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.