Remove ReportPendingException calls in Codegen.py

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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

4 years ago
Component: JavaScript Engine → DOM: Core & HTML
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.
That said, I'm pretty happy to modify the exception-reporting contract for ClearCache*Value as needed.
(Assignee)

Comment 3

4 years ago
Created attachment 8523229 [details] [diff] [review]
remove_rpe_codegen-v0.diff

(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 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

4 years ago
Created attachment 8523304 [details] [diff] [review]
remove_rpe_codegen-v1.diff

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 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

4 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
https://hg.mozilla.org/mozilla-central/rev/19a306c820b9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.