Closed Bug 1149830 Opened 5 years ago Closed 3 years ago

Remove nsIStackFrame::language

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox40 --- affected
firefox57 --- fixed

People

(Reporter: mccr8, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
> if a stack from is JavaScript
That should read "if a stack frame is JavaScript".
> 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.
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.
We could check via addons mxr...
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.
Keywords: addon-compat
I'll dig into this a little.
Assignee: nobody → continuation
This is a little more yak-shave-y than I want to deal with right now.
Assignee: continuation → nobody
Apparently bug 1257919 part 4 killed the last user in Console.cpp. No one refers nsIStackFrame::language anymore.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
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.
Attachment #8893673 - Flags: review?(continuation) → review+
Comment on attachment 8893673 [details]
Bug 1149830 - Remove nsIStackFrame::language.

r=bz for the WebIDL change.
Attachment #8893673 - Flags: review?(bzbarsky)
err... r?bz I mean. :)
Comment on attachment 8893673 [details]
Bug 1149830 - Remove nsIStackFrame::language.

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

r=me
Attachment #8893673 - Flags: review?(bzbarsky) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/331cdaa5807b
Remove nsIStackFrame::language. r=bz,mccr8
https://hg.mozilla.org/mozilla-central/rev/331cdaa5807b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Duplicate of this bug: 1147947
Keywords: addon-compat
(In reply to Andrew McCreight [:mccr8] from comment #0)
> do any addons use this?

Stylish uses this and now it's broken.
> 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...
You need to log in before you can comment on or make changes to this bug.