Closed Bug 1132934 Opened 9 years ago Closed 6 years ago

Consider preventing accidental calls to XPCOM methods from bindings code

Categories

(Core :: DOM: Core & HTML, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 14 obsolete files)

1.98 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Consider this IDL

  interface Foo {
    attribute boolean bar;
  };

and this implementation class:

  class Foo {
    bool Bar();
    NS_IMETHOD SetBar(bool arg);
  };

Currently this compiles just fine and silently ignores the return value of SetBar.  This causes problems like bug 1132347 where we ignore errors from functions we should really care about.

This bug is about maybe preventing that.  I say maybe because a bunch of HTML element things explicily have bindings call XPCOM setters that are known to not fail...  and because the signatures for an attribute setter that takes a string are actually identical for bindings and XPCOM, which means the only way to disambiguate is by renaming one or the other.
I guess in fact that all the cases this would catch have identical signatures, so we'd need to rename a bunch of things that are in fact infallible.  Maybe that's ok.
Depends on: 1431774
Depends on: 1431964
Depends on: 1432186
Depends on: 1434318
Depends on: 1434641
Depends on: 1434818
Depends on: 1434803
Depends on: 1434819
Depends on: 1435132
Depends on: 1435138
Depends on: 1435430
MozReview-Commit-ID: A20gseUzUFT
Attachment #8948087 - Flags: review?(nika)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: L1ILcLGAyy
Attachment #8948088 - Flags: review?(nika)
MozReview-Commit-ID: Csqh40GjqJn
Attachment #8948089 - Flags: review?(nika)
MozReview-Commit-ID: C15Gr1oxq8s
Attachment #8948090 - Flags: review?(nika)
MozReview-Commit-ID: 47Pc4Puub4T
Attachment #8948091 - Flags: review?(nika)
MozReview-Commit-ID: EmoJDwDw4kp
Attachment #8948092 - Flags: review?(nika)
MozReview-Commit-ID: 8cf2jHkGS7L
Attachment #8948093 - Flags: review?(nika)
MozReview-Commit-ID: 3qsXzmoqQlY
Attachment #8948094 - Flags: review?(nika)
MozReview-Commit-ID: PCHylPszxI
Attachment #8948095 - Flags: review?(nika)
MozReview-Commit-ID: JBdem9vRgcN
Attachment #8948096 - Flags: review?(nika)
Depends on: 1435480
Attachment #8948087 - Attachment is obsolete: true
Attachment #8948087 - Flags: review?(nika)
Attachment #8948096 - Attachment is obsolete: true
Attachment #8948096 - Flags: review?(nika)
Attachment #8948095 - Attachment is obsolete: true
Attachment #8948095 - Flags: review?(nika)
Attachment #8948094 - Attachment is obsolete: true
Attachment #8948094 - Flags: review?(nika)
Attachment #8948093 - Attachment is obsolete: true
Attachment #8948093 - Flags: review?(nika)
Attachment #8948092 - Attachment is obsolete: true
Attachment #8948092 - Flags: review?(nika)
Attachment #8948091 - Attachment is obsolete: true
Attachment #8948091 - Flags: review?(nika)
Attachment #8948090 - Attachment is obsolete: true
Attachment #8948090 - Flags: review?(nika)
Attachment #8948089 - Attachment is obsolete: true
Attachment #8948089 - Flags: review?(nika)
Attachment #8948088 - Attachment is obsolete: true
Attachment #8948088 - Flags: review?(nika)
Depends on: 1435483
Depends on: 1436508
Depends on: 1438525
Depends on: 1444586
Attached patch Fix, which can't be landed yet (obsolete) — Splinter Review
Depends on: 1444686
Depends on: 1445140
Depends on: 1445141
Depends on: 1446533
Depends on: 1377980
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1023e033bfa6
part 1.  Actually propagate out exceptions from the innerHTML getter.  r=smaug
Keywords: leave-open
Depends on: 1449019
MozReview-Commit-ID: KEqsx836qQn
Attachment #8962502 - Flags: review?(bugs)
Backout by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/463447801c4c
Backed out changeset 1023e033bfa6 because it hasn't been reviewed yet.
Comment on attachment 8962502 [details] [diff] [review]
Actually propagate out exceptions from the innerHTML getter

I guess we could improve GetMarkup to throw OOM, if needed, in the future.
Attachment #8962502 - Flags: review?(bugs) → review+
Comment on attachment 8962502 [details] [diff] [review]
Actually propagate out exceptions from the innerHTML getter

This was not meant to go on this bug... and it should be using GetterCanOOM anyway.
Attachment #8962502 - Attachment is obsolete: true
Attachment #8962502 - Flags: review+
Depends on: 1455052
Priority: -- → P3
Depends on: 1465875
Depends on: 1466213
Depends on: 1466253
Depends on: 1466298
Depends on: 1466643
There are no JS implementations that I can see, but this will get us nicer bisectability if it turns out I'm wrong.
Attachment #8983164 - Flags: review?(dtownsend)
Attachment #8983165 - Flags: review?(dtownsend)
Attachment #8983164 - Attachment is obsolete: true
Attachment #8983164 - Flags: review?(dtownsend)
Attachment #8983165 - Attachment is obsolete: true
Attachment #8983165 - Flags: review?(dtownsend)
Depends on: 1466673
Depends on: 1466727
Depends on: 1466851
Depends on: 1494524
Attachment #8957787 - Attachment is obsolete: true
Attachment #9012457 - Flags: review?(bugs) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/596f7ad9ff29
Make Web IDL bindings enforce that void methods they call (including setters) actually have void as a return type.  r=smaug
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.