Closed
Bug 1099278
Opened 10 years ago
Closed 10 years ago
Remove ReportPendingException calls in Codegen.py
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
2.14 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Grepping the objdir shows that these calls are only emitted for WindowBinding and TestCodeGenBinding. I guess these are only usable from the main thread, so we once again won't need any additional infrastructure.
Assignee | ||
Updated•10 years ago
|
Component: JavaScript Engine → DOM: Core & HTML
Comment 1•10 years ago
|
||
Well.... This is only used for non-[Constant] StoreInSlot props. Right now those happen to only exist on Window and in the test codegen, but nothing precludes them existing in a worker.
Comment 2•10 years ago
|
||
That said, I'm pretty happy to modify the exception-reporting contract for ClearCache*Value as needed.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #2) > That said, I'm pretty happy to modify the exception-reporting contract for > ClearCache*Value as needed. Yes, I think we need to, although I'm not entirely sure how the current behavior could possibly be valid. Let's look at the existing behavior of nsGlobalWindow::SetNewDocument first. In this method, we have an AutoJSAPI on the stack; we pass the cx from that down to ClearDocumentDependentSlots; this just calls into generated code for clearing each slot; and then squelches any non-true return from those calls. In the generated code, we handle a JSAPI failure by manually reporting the error; thus clobbering it; but then we return false anyway; which is then ignored by the caller. (1) At the very, very least, it would make more sense to clobber the error and the return status at the same level of the stack: e.g. in ClearDocumentDependentSlots. The attached patch does this and leaves the other semantics alone, modulo workers. (2) The thing I don't understand here is why it's correct to leave the Document-dependent properties on the inner window (a) at all and (b) when it was wrapper creation that failed. Either of these conditions seems like it would leak information between compartments, so I would think that a JSAPI failure in the generated code should percolate up and cause navigation to fail or at least crash safely. The attached patch does not address usage from workers or the above issue -- I'd like to know how big a categorical error I've made in my understanding of this code before proceeding.
Attachment #8523229 -
Flags: feedback?(bzbarsky)
Comment 4•10 years ago
|
||
Comment on attachment 8523229 [details] [diff] [review] remove_rpe_codegen-v0.diff So... I dearly wish we could just make OOM fatal here (which is the only props that can fail). I think doing a runtime abort in ClearDocumentDependentSlots if clearing fails in this case makes sense to me. >+ // that we can squash and non-fatal errors here. "and non-fatal" doesn't make sense to me.
Attachment #8523229 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
If we don't need to handle the error, then we don't need to make TakeOwnershipOfErrorReporting handle workers for this bug either. Unfortunately NS_ABORT_OOM wants a size and we have no idea at the point where we determine that we can't actually handle the error, so I've just used MOZ_CRASH directly.
Attachment #8523229 -
Attachment is obsolete: true
Attachment #8523304 -
Flags: review?(bzbarsky)
Comment 6•10 years ago
|
||
Comment on attachment 8523304 [details] [diff] [review] remove_rpe_codegen-v1.diff r=me Can you please update the documentation at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#StoreInSlot to make it clear what the error handling contract is here?
Attachment #8523304 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Please do not ask for reviews for a bit [:bz] from comment #6) > Can you please update the documentation at > https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#StoreInSlot > to make it clear what the error handling contract is here? Done. https://hg.mozilla.org/integration/mozilla-inbound/rev/19a306c820b9
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/19a306c820b9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•