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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
2 years ago
2 years 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)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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
(Assignee)

Comment 3

2 years ago
Created attachment 8791143 [details] [diff] [review]
doc3.patch
Assignee: nobody → amarchesini
Attachment #8791143 - Flags: review?(n.nethercote)
(Reporter)

Comment 4

2 years ago
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+
(Assignee)

Updated

2 years ago
Attachment #8791143 - Flags: review?(bugs)

Comment 5

2 years ago
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-
(Assignee)

Comment 6

2 years ago
Created attachment 8791636 [details] [diff] [review]
doc3.patch
Attachment #8791143 - Attachment is obsolete: true
Attachment #8791636 - Flags: review?(bugs)

Comment 7

2 years ago
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+

Comment 8

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/553eb2fc3a71
Handle GetSpec() failure in nsContentUtils::GetWrapperSafeScriptFilename(), r=smaug

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/553eb2fc3a71
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.
status-firefox51: affected → fix-optional
You need to log in before you can comment on or make changes to this bug.