new Request("",{redirect:"foo"}) in web console causes tab crash

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ziyunfei, Assigned: mrrrgn)

Tracking

({regression})

48 Branch
mozilla49
regression
Points:
---

Firefox Tracking Flags

(firefox46 unaffected, firefox47 unaffected, firefox48+ fixed, firefox49+ fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8749530 [details]
nightly 2016-05-05

new Request("",{redirect:"foo"})
Confirmed.  Thank you for the report!
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
I investigated with Boris in IRC for a bit.  It seems this only crashes when run from the console, but not a page.  Apparently the console is doing something unexpected with the error result.

The Debugger.cpp should probably null-check that efs pointer, though:

https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#8188
Assignee: bkelly → bzbarsky
Summary: new Request("",{redirect:"foo"}) causes tab crash → new Request("",{redirect:"foo"}) in web console causes tab crash
I can only reproduce in FF48 and FF49.
status-firefox46: --- → unaffected
status-firefox47: --- → unaffected
status-firefox48: --- → affected
status-firefox49: --- → affected
[Tracking Requested - why for this release]:  Crashes when people try to debug in the console

This is a regression from bug 1261904.

The bindings throw an exception in this case by doing:

  JS_ReportErrorNumberVA(aCx, GetErrorMessage, nullptr,
                         static_cast<const unsigned>(aErrorNumber), ap);

where GetErrorMessage is the one defined in dom/bindings/BindingUtils.cpp.  Note that per the JS_ReportErrorNumberVA API, the fourth arg (error number) is something that only the passed-in second arg knows what to do with.  In this case, it looks it up in the array of DOM error messages.  We're passing MSG_INVALID_ENUM_VALUE, which is 0.

Then the debugger code gets its hands on the resulting exception and does this:

8172        JSErrorReport* report = nullptr;
8173        if (obj->is<ErrorObject>())
8174            report = obj->as<ErrorObject>().getErrorReport();

and gets a non-null JSErrorReport.  Then it does:

8176        if (report) {
8177            const JSErrorFormatString* efs = GetErrorMessage(nullptr, report->errorNumber);
8178            RootedString str(cx, JS_NewStringCopyZ(cx, efs->name));

That right there is fail.  This is calling js::GetErrorMessage and passing it an error number that was meant for mozilla::dom::GetErrorMessage.  That will not work right.  In this case it crashes, because 0 is JSMSG_NOT_AN_ERROR as far as js::GetErrorMessage goes, and hence efs ends up null.

If we're going to really be trying to get the original error message, we should be using the original callback function that was meant to be used with it.  Or something.  It's not quite clear to me what this code is really trying to do, since error message names, like error message numbers, have absolutely zero guarantee of being unique...

Adding a nullcheck would help for the particular case of the error number being 0, in terms of not crashing, but for other error numbers you would just end up with the wrong error name.  So nullchecking isn't really the right fix here.
Assignee: bzbarsky → winter2718
Blocks: 1261904
status-firefox46: unaffected → ---
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
Component: DOM → JavaScript Engine
I guess the docs in Debugger.Object.md say:

 :  If the referent is an error created with an engine internal message template
    this is a string which is the name of the template; `undefined` otherwise.

The problem is that in this case it's an error NOT created with an engine internal message template, but we act as if it were.
Hmm.  Looks like the code was already broken, and just got moved.  Is it just that console didn't use to trigger it but now it does?
Flags: needinfo?(winter2718)
Flags: needinfo?(jimb)
In particular, of someone is just enumerating all props on the debugger object for the exception, they will now hit this whereas they didn't use to.
status-firefox46: --- → unaffected
Keywords: regression
Maybe the right proximate fix is to change JSErrorReport from storing "unsigned errorNumber" to storing "JSErrorFormatString*"?  Or storing both?  Or storing the error number and the callback function pointer, which can then be used to get the JSErrorFormatString*.
(Assignee)

Comment 9

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> Maybe the right proximate fix is to change JSErrorReport from storing
> "unsigned errorNumber" to storing "JSErrorFormatString*"?  Or storing both? 
> Or storing the error number and the callback function pointer, which can
> then be used to get the JSErrorFormatString*.

Adding JSErrorFormatString* seems like a nice way to distinguish between JS Engine and Dom errors here.
Flags: needinfo?(winter2718)
The DOM errors would have a JSErrorFormatString* too.  It would just point to the DOM ones at http://hg.mozilla.org/mozilla-central/file/0177462aac74/dom/bindings/BindingUtils.cpp#l58
(Assignee)

Comment 11

2 years ago
Addressing the symptom with a patch to check for a null efs. To fix the root problem I've filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1270918
(Assignee)

Comment 12

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #10)
> The DOM errors would have a JSErrorFormatString* too.  It would just point
> to the DOM ones at
> http://hg.mozilla.org/mozilla-central/file/0177462aac74/dom/bindings/
> BindingUtils.cpp#l58

I see now, that makes even more sense.
(Assignee)

Comment 13

2 years ago
Created attachment 8749759 [details] [diff] [review]
efscrash.diff
Attachment #8749759 - Flags: review?(jimb)

Updated

2 years ago
Attachment #8749759 - Flags: review?(jimb) → review+

Comment 14

2 years ago
That accessor is pretty new. I didn't understand the relationship between error numbers and the GetErrorMessage function. The code that misinterprets the error numbers seems to have been present in the original method, which was added in Bug 1245877.
Flags: needinfo?(jimb)
Note that this patch needs to be backported to Aurora, at least.  Maybe beta if we think the method from bug 1245877 can be hit there.
(Assignee)

Comment 17

2 years ago
Comment on attachment 8749759 [details] [diff] [review]
efscrash.diff

Approval Request Comment
[Feature/regressing bug #]: 1270721
[User impact if declined]: Browser crashes.
[Describe test coverage new/current, TreeHerder]: A test is included in the patch.
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8749759 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

2 years ago
Comment on attachment 8749759 [details] [diff] [review]
efscrash.diff

Approval Request Comment
[Feature/regressing bug #]: 1270721
[User impact if declined]: Browser crashes.
[Describe test coverage new/current, TreeHerder]: A test is included in the patch.
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8749759 - Flags: approval-mozilla-beta?

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df6140223c03
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8749759 [details] [diff] [review]
efscrash.diff

If Fx47 is unaffected, we should not be uplifting to Beta.
Attachment #8749759 - Flags: approval-mozilla-beta?
Tracking for 48 and 49 since this is a regression that can a crash.
tracking-firefox48: ? → +
tracking-firefox49: ? → +
Comment on attachment 8749759 [details] [diff] [review]
efscrash.diff

Crash fix, includes a test, sounds good for uplift.
Attachment #8749759 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2dee0c9ad1fb
status-firefox48: affected → fixed
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.