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

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: njn, Assigned: baku)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox51 fix-optional, firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Posted patch doc3.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8791143 - Flags: review?(n.nethercote)
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.
Attachment #8791143 - Flags: review?(n.nethercote) → feedback+
Attachment #8791143 - Flags: review?(bugs)
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.
Attachment #8791143 - Flags: review?(bugs) → review-
Posted patch doc3.patchSplinter Review
Attachment #8791143 - Attachment is obsolete: true
Attachment #8791636 - Flags: review?(bugs)
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.
Attachment #8791636 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/553eb2fc3a71
Handle GetSpec() failure in nsContentUtils::GetWrapperSafeScriptFilename(), r=smaug
https://hg.mozilla.org/mozilla-central/rev/553eb2fc3a71
Status: NEW → RESOLVED
Closed: 3 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.