1094825 - Log use of window.crypto.signText in web console

Status: RESOLVED WONTFIX

(DevTools :: Console, defect)

Description

[Mark Goodwin [:mgoodwin]]

window.crypto.signText is going away soon.

Let's let developers know about this (and what to do instead) in the web console.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1094825_console_message.patch

This is the deprecation warning for signText for Fx34.

Comment 2

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8521499 [details] [diff] [review]
Bug1094825_console_message.patch

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

r=jcj

Looks straightforward and safe.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8521499 [details] [diff] [review]
Bug1094825_console_message.patch

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

Looks good. r=me with comments addressed.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2458,5 @@
> +  nsCOMPtr<nsIDocument> document;
> +  GetDocumentFromContext(aContext, getter_AddRefs(document));
> +  if (!document) {
> +    aReturn.Append(internalError);
> +

nit: probably don't need this blank line (oh, I see - it was like that already. Well, go ahead and remove the blank lines anyway.)

@@ +2463,5 @@
> +    return;
> +  }
> +
> +  nsIURI* uri = document->GetDocumentURI();
> +

nit: same

@@ +2466,5 @@
> +  nsIURI* uri = document->GetDocumentURI();
> +
> +  if (!uri) {
> +    aReturn.Append(internalError);
> +

nit: same

@@ +2470,5 @@
> +
> +    return;
> +  }
> +
> +  nsContentUtils::ReportToConsoleNonLocalized(NS_LITERAL_STRING("window.crypto.signText is deprecated and will soon be removed."),

Maybe make a named literal string instead of having this line be long. Also, should we provide a link to documentation on this?

@@ +2471,5 @@
> +    return;
> +  }
> +
> +  nsContentUtils::ReportToConsoleNonLocalized(NS_LITERAL_STRING("window.crypto.signText is deprecated and will soon be removed."),
> +                                          nsIScriptError::errorFlag,

The indentation here should match up with the ( on the previous line.

@@ +2480,1 @@
>    aReturn.Truncate();

This should go before any uses of aReturn, I believe.

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #3)
> >    aReturn.Truncate();
> 
> This should go before any uses of aReturn, I believe.

I think the intent was for internalError and the truncation of aReturn to be together - but the latter looks misplaced with my additions in between. I think the right thing to do here is to move aReturn.Truncate() above my additions. Agreed?

Comment 5

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1094825_console_message.patch

Sorry to be a pain, Panos, but Keeler suggested a '[learn more]'.

The Devtoolsy bit is now a hefty 5 lines...

Comment 6

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8521679 [details] [diff] [review]
Bug1094825_console_message.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1094825. See also https://bugzilla.mozilla.org/show_bug.cgi?id=1083118#c20

[User impact if declined]:
Developers will have no warning of impending removal of window.crypto.signText

[Describe test coverage new/current, TBPL]:
Current test coverage is adequate for devtools changes. There is no current or new coverage of window.crypto.signText (this is a deprecation warning for this function)

[Risks and why]: 
Low risk; there's a minimal change to nsCrypto.cpp (of local significance) and an even smaller change to webconsole.js which has good test coverage.

[String/UUID change made/needed]:
No String or UUID changes. There is a (non-localized) web console message warning of deprecation and a link to MDN content for more info.

Comment 7

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=14c5bbc6e9bd

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8521679 [details] [diff] [review]
Bug1094825_console_message.patch

I understand the reasoning behind this but without knowing whether this will be effective or not it seems too late in Beta to take this change. People should already have been alerted to the fact that crypto.signText() is going away by our disabling the feature in Firefox 33. We will also be talking about this and need to ensure people know about our migration path after the 34 release. How about adding the warning to the dev browser (currently 35), which is what we are suggesting developers use?

Comment 9

[Mark Goodwin [:mgoodwin]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> How about adding the warning to the dev browser (currently 35),
> which is what we are suggesting developers use?

My understanding is that the plan was for this to be gone (again) in 35.

Is that correct, Richard?

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

signText is gone in 35 - this isn't necessary anymore.