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)
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)
93.23 KB,
image/png
|
Details | |
2.27 KB,
patch
|
jimb
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
new Request("",{redirect:"foo"})
Comment 1•8 years ago
|
||
Confirmed. Thank you for the report!
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
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
Comment 3•8 years ago
|
||
I can only reproduce in FF48 and FF49.
status-firefox46:
--- → unaffected
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Comment 4•8 years ago
|
||
[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
Comment 5•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
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.
Updated•8 years ago
|
status-firefox46:
--- → unaffected
Keywords: regression
Comment 8•8 years ago
|
||
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•8 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)
Comment 10•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Attachment #8749759 -
Flags: review?(jimb)
Updated•8 years ago
|
Attachment #8749759 -
Flags: review?(jimb) → review+
Comment 14•8 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)
Comment 16•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
bugherder |
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?
Comment 22•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/2dee0c9ad1fb
Updated•8 years ago
|
Version: unspecified → 48 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•