1173638 - Make creation of an Error not capture a filename/lineno it shouldn't have access to

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Right now Error construction grabs filename/lineno from a frameiter directly.  We should get it from our saved stack, which will automatically do the right thing.

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

OK, so this sucks.

I have a kinda sorta patch but is has several problems:

1) It will capture filename/lineno from self-hosted code if you do this via an Xray from chrome:

  content.Array.indexOf()

That's because unlike PopulateReportBlame the savedstacks code doesn't use NonBuiltinFrameIter.  Should it, perhaps?

2) In the JS_ReportWhatever case, I have no sane way to populate the JSErrorReport's filename from the stack.  So I really need to keep using PopulateReportBlame.  Can we make that skip over stackframes cx doesn't subsume, perhaps?

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: What I have so far

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Not doing reviews right now from comment #1)
> OK, so this sucks.
> 
> I have a kinda sorta patch but is has several problems:
> 
> 1) It will capture filename/lineno from self-hosted code if you do this via
> an Xray from chrome:
> 
>   content.Array.indexOf()
> 
> That's because unlike PopulateReportBlame the savedstacks code doesn't use
> NonBuiltinFrameIter.  Should it, perhaps?

One of our goals was to capture everything and keep a complete record of the stack, and then filter information given out depending on who is asking.

We could certainly add a flag to skip self-hosted frames to SavedFrame/JSAPI methods. Would that work?

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> and then filter information given out depending on who is asking.

Oh, we filter the information out in BuildStackString (unconditionally, even, unlike NonBuiltinFrameIter).  Just not anywhere else in saved stacks.

Fwiw, given item #2 above I'm just going to revert most of what I'm doing and change PopulateReportBlame to principal-filter.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Not doing reviews right now from comment #4)
> > and then filter information given out depending on who is asking.
> 
> Oh, we filter the information out in BuildStackString (unconditionally,
> even, unlike NonBuiltinFrameIter).  Just not anywhere else in saved stacks.

Well, we always filter on principal, which is what I was referring to by "who is asking".

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Right.  It's just weird to me to include stuff in .source/lineNumber/columnNumber that's never in the stack string.

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: Make the constructors for Error and its subtypes get their filenames/linenumbers from the saved stack, not manually

Bobby, could you review the webgl and xpconnect changes?

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8621386 [details] [diff] [review]
Make the constructors for Error and its subtypes get their filenames/linenumbers from the saved stack, not manually

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

::: dom/canvas/WebGLContextUtils.cpp
@@ +498,5 @@
> +
> +    nsIGlobalObject* global = mCanvasElement->OwnerDoc()->GetScopeObject();
> +    if (!global) {
> +        return;
> +    }

AutoJSAPI::Init null-checks the arg, so you can remove this and just inline the whole mCanvasElement->OwnerDoc()->GetScopeObject() into the .Init call.

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +580,5 @@
>        }
>        testProperty('message', x => x == 'some message', 'some other message', 42);
> +      testProperty('fileName', x => x == '', 'otherFilename.html', new iwin.Object());
> +      testProperty('columnNumber', x => x == 1, 99, 99.5);
> +      testProperty('lineNumber', x => x == 0, 50, 'foo');

Hm, why is this the right thing? I recognize that the exception is getting created in the content compartment, but the first frame of the JS stack should be the .xul one, and presumably we should see that here over Xrays, right?

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> Hm, why is this the right thing?

Because there is only chrome code on the stack.  Since the exception is created in the content compartment, and we want to be able to safely expose it to content code in bug 1173593, that means the exception needs to look like the stack was empty when it was created.

> and presumably we should see that here over Xrays, right

Currently Error objects have no provisions for exposing different values of fileName/lineNumber/columnNumber to different consumers.  So we end up with the observed behavior.

Note that this test is already testing that .stack is empty, by the way... at least now our fileName/columnNumber/lineNumber are consistent with .stack.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #9)
> > and presumably we should see that here over Xrays, right
> 
> Currently Error objects have no provisions for exposing different values of
> fileName/lineNumber/columnNumber to different consumers.  So we end up with
> the observed behavior.
> 
> Note that this test is already testing that .stack is empty, by the way...
> at least now our fileName/columnNumber/lineNumber are consistent with .stack.

Ok. But it seems like this is moving us in the wrong direction, right? Isn't the goal with saved stack information to save complete information, and then expose the appropriate APIs for callers to principal filter at the right time?

I'm not necessarily objecting to whatever temporary measures you're implementing, but it seems like there needs to be a discussion and a bug filed for whatever the proper solution looks like, which I don't see right now (but maybe it's somewere somewhere else).

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> Isn't the goal with saved stack information to save complete information, and then expose
> the appropriate APIs for callers to principal filter at the right time?

Yes.  That requires a bunch of surgery on not only Error objects but also JSErrorReport.  See comment 1.  It's way more work than I want to sign up for right now.  So for now I'm fixing what's left of bug 1127694, at least.

I thought we had a bug on that already myself, but I'm not seeing one.  I filed bug 1174133.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8621386 [details] [diff] [review]
Make the constructors for Error and its subtypes get their filenames/linenumbers from the saved stack, not manually

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

LGTM

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8621386 [details] [diff] [review]
Make the constructors for Error and its subtypes get their filenames/linenumbers from the saved stack, not manually

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

r=me modulo comment 8.

Comment 14

[Boris Zbarsky [:bzbarsky, bz on IRC]]

You mean the first part of comment 8?

Comment 15

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #14)
> You mean the first part of comment 8?

Yes.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fde58d44a4c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fde58d44a4c