1149830 - Remove nsIStackFrame::language

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

This is the last remaining user of nsIProgrammingLanguage (once I land my other patches).  I think the only use of this is in console code, to check if a stack from is JavaScript.  This would be easy to replace with some kind of isJavaScript() method or field.  The main question is, do any addons use this?  This seems very debugger-ish, so maybe Firebug is the only real danger.

The languageName field looks entirely unused in the tree, so maybe that could be removed, too.

Comment 1

[Andrew McCreight [:mccr8]]

> if a stack from is JavaScript
That should read "if a stack frame is JavaScript".

Comment 2

[Boris Zbarsky [:bzbarsky]]

> This would be easy to replace with some kind of isJavaScript() method or field.

So...  The console uses this in two places: Method() and ReifyStack.  Both get their stack from CreateStack.  CreateStack only puts JSStackFrame and StackFrame objects in there.  mLanguage is JavaScript for JSStackFrame and UNKNOWN for StackFrame.

Now here's the thing: the only thing StackFrame is used for is to indicate "empty stack".

OK, so who are the consumers of CreateStack?  There's the console code and also GetCurrentJSStack.  As far as I can tell, that's it.  GetCurrentJSStack is called from 3 places:

1)  Exception constructor.
2)  nsXPConnect::GetCurrentJSStack
3)  SandboxLogJSStack.

#3 will ignore the non-JS frame already.  #2 is used from:

i)   Exception::Initialize
ii)  GetJSStackForBlob
iii) nsXPCComponents_Utils::ReportError
iv)  nsXPCComponents_Utils::EvalInSandbox
v)   AssembleSandboxMemoryReporterName(

(ii)-(iv) all treat null and having a non-JS stackframe identically, afaict.  (v) will produce some bogus string from it: "(from: :0)".  (i) is the same as #1 above, really.

So the real question is what the difference is between an Exception that has null as the location (which, btw, can happen anyway if there is no current JSContext in GetCurrentJSStack) and which has a single non-JS stackframe as location.  I suspect the answer is "not much of a difference" and we should just return null when there is no JS on the stack and kill off StackFrame.

Also, the mFilename member of Exception seems to always be an empty string....  and mLineNumber is never useful either.

I guess the point is we can clean all this this up a good bit if we're willing to invest a bit of time in it.

Comment 3

[Andrew McCreight [:mccr8]]

Do you think we need to worry about Firebug or somebody using any of this or is that too weird?  I guess none of our own devtools even seem to use it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We could check via addons mxr...

Comment 5

[Andrew McCreight [:mccr8]]

I found a user of nsIStackFrame::language, "Sepsis console":
  https://addons.mozilla.org/en-US/firefox/addon/sepsis-console/?src=search
It uses it here: (requires AMO MXR access)
  https://mxr.mozilla.org/addons/source/431538/components/ConsoleAPI.js#481
It is similar to the use in our console code.  That addon has a max version of 32, so maybe it doesn't work anyways.

There's also a number of uses in the FirefoxOS 1.1 and 1.2 simulators, which seem to include the old Console code implemented in JS, but I think we can ignore that.

Comment 6

[Andrew McCreight [:mccr8]]

I'll dig into this a little.

Comment 7

[Andrew McCreight [:mccr8]]

This is a little more yak-shave-y than I want to deal with right now.

Comment 8

[Masatoshi Kimura [:emk]]

Apparently bug 1257919 part 4 killed the last user in Console.cpp. No one refers nsIStackFrame::language anymore.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Bug 1149830 - Remove nsIStackFrame::language.

Review commit: https://reviewboard.mozilla.org/r/164768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164768/

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8893673 [details]
Bug 1149830 - Remove nsIStackFrame::language.

https://reviewboard.mozilla.org/r/164768/#review170398

Thanks! This looks fine to me, except for the WebIDL change which I don't know anything about. You also need a DOM peer review for that. Maybe you could ask Boris to take a look.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8893673 [details]
Bug 1149830 - Remove nsIStackFrame::language.

r=bz for the WebIDL change.

Comment 12

[Andrew McCreight [:mccr8]]

err... r?bz I mean. :)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8893673 [details]
Bug 1149830 - Remove nsIStackFrame::language.

https://reviewboard.mozilla.org/r/164768/#review170410

r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/331cdaa5807b
Remove nsIStackFrame::language. r=bz,mccr8

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/331cdaa5807b

Comment 17

[Oriol Brufau [:Oriol]]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> do any addons use this?

Stylish uses this and now it's broken.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Stylish uses this

I just downloaded https://addons.mozilla.org/firefox/downloads/latest/stylish/type:attachment/addon-2108-latest.xpi and unzipped it.  It doesn't use "language" anywhere.

What it _does_ use is Components.interfaces.nsIProgrammingLanguage.JAVASCRIPT and that's now presumably throwing exceptions.  It's using that to implement nsIClassInfo.implementationLanguage, which hasn't been a thing since Firefox 40 (see bug 1147572).  So just removing those two "implementationLanguage: Components.interfaces.nsIProgrammingLanguage.JAVASCRIPT" will fix things.

Of course Stylish is not compatible with Firefox 57 anyway at the moment...