1618011 - Give binding error messages a consistent prefix

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), task, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we have a consistent prefix (the function name) for error messages thrown via ErrorResult to bindings, but not for error messages thrown from the bindings themselves. It would be good to fix this. Working on it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: File exercising various exceptions and showing the messages for them before and after this patch stack

The new ones are better, or at least I think so. ;)

The patches I have right now add about 150KB of codesize. I'm going to measure what happens to the codesize if we use explicit string passing instead of bundling up the JSContext* and string in a BindingCallContext.

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, I tried the "pass strings around" approach and it seems to do better on codesize: it only increases .text by 49KB or so. In addition to that, it decreases .rodata by 55KB (as compared to 24KB with the BindingCallContext approach) and only increases .eh_frame and .eh_frame_hdr by about 12KB, compared to 30KB for BindingCallContext. So net it's an increase of 49KB of code and net decrease of 42KB of readonly data (as compared to an increase of 150KB of code and net increase of 7KB of readonly data).

I can improve the readonly data bits for the BindingCallContext stuff by applying the ": " handling from the other patches to it, but I'm not sure it's worth spending time on that unless we plan to use those patches.

The BindingCallContext version does have the benefit of being slightly simpler, I think, and not being quite as fragile wrt unused variable warnings. Avoiding those means either getting the set of cases when we need a method description variable exactly right (which at this point I think I have) or using literal strings instead that then get threaded through the codegen instead of a variable at the top of the method.

Also, there are some cases where a binding method has a methodDescription string and an identical string in the MaybeSetPendingException call. I think compilers will coalesce those anyway, so I didn't worry about manually trying to coalesce them.

I'm going to post my BindingCallContext patches here, just so they don't get lost, then work on posting the string-passing patches for review in Phabricator. Still waiting on the try runs for those.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Nathan, if you find a convenient way to measure the ARM codesize, or can just do it locally, I am happy to provide rollup patches here too...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a BindingCallContext class

This class encapsulates a JSContext and a method description, and will let us
incrementally convert the various places that currently throw exceptions via
ThrowErrorMessage to including that method description, by converting the
relevant code from taking JSContext* to taking BindingCallContext.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use a BindingCallContext for unions

Unions can always throw MSG_NOT_IN_UNION, so always need a BindingCallContext.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use a BindingCallContext for dictionaries

Dictionaries always need a BindingCallContext, because they can throw
MSG_NOT_DICTIONARY from Init().

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Use a BindingCallContext for Web IDL callback guts

We basically need this because some of the type conversions those guts need to
do may need a BindingCallContext.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Use a BindingCallContext for records

These can always throw a "not an object" exception.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Use a BindingCallContext for primitive conversions as needed

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Make idlTypeNeedsCallContext return true for any type that can do ThrowErrorMessage

This will let us switch those over to BindingCallContext later.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Switch most error messages used in bindings to having a method name prefix

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Switch ByteString conversions to having a method name prefix

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Pass a source description to primitive conversions

We can use it for error reporting.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Make error reporting for unions nicer

Instead of something like
"ByteStringSequenceSequenceOrByteStringByteStringRecord" we should have
"(sequence<sequence<ByteString>> or record<ByteString, Bytestring>)" in error
messages.

Please take a careful look at the resulting error messages (see attachment
in the bug) to see whether this makes sense.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Use a BindingCallContext for initializing AudioParamDescriptors

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 1. Implement support for error context in ThrowErrorMessage.

This does not change behavior at the moment, because the only callers of
ThrowErrorMessage that pass an error number that has a context pass an empty
string for the first (context) arg. Both of those callers are changed to pass
nullptr for the context in this patch.

We want to support nullptr to mean "empty context", because that way at
callsites we can avoid having extra empty strings.

We could avoid putting this machinery in place if we hardcoded the trailing ":
" at all the callsites, but that would reduce future flexibility in where the
context is placed in the message string (e.g. if we wanted to move it to the
end instead of the beginning) and increase the amount of string data we have to
cart around in the binary quite noticeably: we have a lot of places in
bindings where we call ThrowErrorMessage.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 2. Add a BindingCallContext class.

Future patches will pass these method descriptions to the places where we might
need them to call ThrowErrorMessage.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 3. Pass a BindingCallContext to union setters.

There are a few places where we pass null temporarily just so things compile;
future patches will fix those to pass something useful.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 4. Pass a BindingCallContext to dictionary Init() methods.

To catch cases which were passing the optional sourceDescription argument, we
define two overloads of Init(): one which takes neither description and one
which requires both of them.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 5. Use a BindingCallContext for Web IDL callback guts.

There's a bit of hoop-jumping here to only declare methodDescription when it'll
actually be used, so we don't trigger unused variable warnings which break
-Werror compiles.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 6. Pass a BindingCallContext (if neded) and source description to primitive conversions.

The method description is something like "InterfaceName.methodName", and the
sorce description is how the primitive is involved in that method
(e.g. "argument 5").

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 7. Switch most error messages used in bindings to having a method name prefix.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 8. Switch ByteString conversions to having a method name prefix.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 9. Make error reporting for unions nicer.

Instead of something like
"ByteStringSequenceSequenceOrByteStringByteStringRecord" we should have
"(sequence<sequence<ByteString>> or record<ByteString, Bytestring>)" in error
messages.

Please take a careful look at the resulting error messages (see attachment
in the bug) to see whether this makes sense.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 10. Use a BindingCallContext for initializing AudioParamDescriptors.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, the codesize numbers were without LTO. I guess I should measure with LTO, just in case....

Comment 27

[Boris Zbarsky [:bzbarsky]]

Well, with LTO the numbers are much closer to each other. Which makes sense, because LTO can get rid of a bunch of the BindingCallContext helper methods that are unused.

Further, with LTO the numbers seem to be within the noise level (LTO builds from the same source have codesize swings that are about the same size as the observed differences).

So I'm going to switch back to the BindingCallContext approach, because it's more maintainable and imo simpler.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1618011 part 11. Don't expose method names like __namedsetter to the web.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9119aeb72ea4
part 1.  Implement support for error context in ThrowErrorMessage. r=peterv
https://hg.mozilla.org/integration/autoland/rev/278a213e5304
part 2.  Add a BindingCallContext class. r=peterv
https://hg.mozilla.org/integration/autoland/rev/85650ee945c4
part 3.  Pass a BindingCallContext to union setters. r=peterv
https://hg.mozilla.org/integration/autoland/rev/b2dfa2e66d24
part 4.  Pass a BindingCallContext to dictionary Init() methods. r=peterv
https://hg.mozilla.org/integration/autoland/rev/29a9227d08ac
part 5.  Use a BindingCallContext for Web IDL callback guts. r=peterv
https://hg.mozilla.org/integration/autoland/rev/433fdf04058c
part 6.  Pass a BindingCallContext (if neded) and source description to primitive conversions. r=peterv
https://hg.mozilla.org/integration/autoland/rev/b01f8c66110b
part 7.  Switch most error messages used in bindings to having a method name prefix. r=peterv
https://hg.mozilla.org/integration/autoland/rev/8b8c4c60c34b
part 8.  Switch ByteString conversions to having a method name prefix. r=peterv
https://hg.mozilla.org/integration/autoland/rev/c50121035d50
part 9.  Make error reporting for unions nicer. r=peterv
https://hg.mozilla.org/integration/autoland/rev/11df2f359473
part 10.  Use a BindingCallContext for initializing AudioParamDescriptors. r=padenot
https://hg.mozilla.org/integration/autoland/rev/8b11ddd8999f
part 11.  Don't expose method names like __namedsetter to the web. r=peterv

Comment 30

[Arthur Iakab [arthur_iakab]]

Backed out 11 changesets (bug 1618011)for Linting failure.

Backout revision https://hg.mozilla.org/integration/autoland/rev/b5c15277450eda187610b12df8abba5981bcbaf7

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=292053049&resultStatus=testfailed%2Cbusted%2Cexception&revision=8b11ddd8999f8189a2834ba200636cd8c08d599a&searchStr=linting%2Copt%2Csource-test-webidl-test%2C%28wp%29

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=292053049&repo=autoland

Boris can you please take a look?

Comment 31

[Boris Zbarsky [:bzbarsky]]

I can take a look, but I did a try push for these exact changes before I pushed them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31c7ee2bd97e1b80413927505e4d4faac9e93c49

Does that lint job not run by default?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Looks like that's the webidl parser tests. Easy fix. Wish try would run them.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4617ca707005
part 1.  Implement support for error context in ThrowErrorMessage. r=peterv
https://hg.mozilla.org/integration/autoland/rev/98fff114a273
part 2.  Add a BindingCallContext class. r=peterv
https://hg.mozilla.org/integration/autoland/rev/5e6bd14f80f9
part 3.  Pass a BindingCallContext to union setters. r=peterv
https://hg.mozilla.org/integration/autoland/rev/d255e3548e0d
part 4.  Pass a BindingCallContext to dictionary Init() methods. r=peterv
https://hg.mozilla.org/integration/autoland/rev/a2ae77d44d02
part 5.  Use a BindingCallContext for Web IDL callback guts. r=peterv
https://hg.mozilla.org/integration/autoland/rev/ceb5d82e68bc
part 6.  Pass a BindingCallContext (if neded) and source description to primitive conversions. r=peterv
https://hg.mozilla.org/integration/autoland/rev/abc482029dfe
part 7.  Switch most error messages used in bindings to having a method name prefix. r=peterv
https://hg.mozilla.org/integration/autoland/rev/f77f2be72d2b
part 8.  Switch ByteString conversions to having a method name prefix. r=peterv
https://hg.mozilla.org/integration/autoland/rev/e665071e5de0
part 9.  Make error reporting for unions nicer. r=peterv
https://hg.mozilla.org/integration/autoland/rev/825f196932d1
part 10.  Use a BindingCallContext for initializing AudioParamDescriptors. r=padenot
https://hg.mozilla.org/integration/autoland/rev/e2dfeee4b34a
part 11.  Don't expose method names like __namedsetter to the web. r=peterv

Comment 34

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4617ca707005
https://hg.mozilla.org/mozilla-central/rev/98fff114a273
https://hg.mozilla.org/mozilla-central/rev/5e6bd14f80f9
https://hg.mozilla.org/mozilla-central/rev/d255e3548e0d
https://hg.mozilla.org/mozilla-central/rev/a2ae77d44d02
https://hg.mozilla.org/mozilla-central/rev/ceb5d82e68bc
https://hg.mozilla.org/mozilla-central/rev/abc482029dfe
https://hg.mozilla.org/mozilla-central/rev/f77f2be72d2b
https://hg.mozilla.org/mozilla-central/rev/e665071e5de0
https://hg.mozilla.org/mozilla-central/rev/825f196932d1
https://hg.mozilla.org/mozilla-central/rev/e2dfeee4b34a