Closed Bug 1173638 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
Flags: needinfo?(nfitzgerald)
Attached patch What I have so far (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(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?
Flags: needinfo?(nfitzgerald)
> 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.
(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".
Right.  It's just weird to me to include stuff in .source/lineNumber/columnNumber that's never in the stack string.
Bobby, could you review the webgl and xpconnect changes?
Attachment #8621386 - Flags: review?(nfitzgerald)
Attachment #8621386 - Flags: review?(bobbyholley)
Attachment #8620784 - Attachment is obsolete: true
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?
Attachment #8621386 - Flags: review?(bobbyholley)
> 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.
Flags: needinfo?(bobbyholley)
(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).
Flags: needinfo?(bobbyholley)
> 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.
Flags: needinfo?(bobbyholley)
Blocks: 1127694
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
Attachment #8621386 - Flags: review?(nfitzgerald) → review+
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.
Attachment #8621386 - Flags: review+
Flags: needinfo?(bobbyholley)
You mean the first part of comment 8?
Flags: needinfo?(bobbyholley)
(In reply to Not doing reviews right now from comment #14)
> You mean the first part of comment 8?

Yes.
Flags: needinfo?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/1fde58d44a4c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.