Closed Bug 1270721 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

48 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 + fixed
firefox49 + fixed

People

(Reporter: 446240525, Assigned: mrrrgn)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image 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.
[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
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.
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*.
(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
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
(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.
Attached patch efscrash.diffSplinter Review
Attachment #8749759 - Flags: review?(jimb)
Attachment #8749759 - Flags: review?(jimb) → review+
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.
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?
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?
https://hg.mozilla.org/mozilla-central/rev/df6140223c03
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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.
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+
Version: unspecified → 48 Branch
You need to log in before you can comment on or make changes to this bug.