1173830 - Errors sent by WebChannel.jsm->_sendErrorEventToContent are not sent to content

Status: RESOLVED DUPLICATE of bug 1320189

(Toolkit :: General, defect)

Description

[Shane Tomlinson [:stomlinson]]

Found while researching the cause of https://github.com/mozilla/fxa-content-server/issues/2518

WebChannel.jsm->_sendErrorEventToContent sents a WebChannelMessageToContent message to content with `id` and `error` fields [1]. Unfortunately, content.js only relays the `id` and `message` fields, `error` is dropped.


[1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/WebChannel.jsm#121
[2] - https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#327

Comment 1

[Vlad Filippov (:vladikoff)]

(In reply to Shane Tomlinson [:stomlinson] from comment #0)
> Found while researching the cause of
> https://github.com/mozilla/fxa-content-server/issues/2518
> 
> WebChannel.jsm->_sendErrorEventToContent sents a WebChannelMessageToContent
> message to content with `id` and `error` fields [1]. Unfortunately,
> content.js only relays the `id` and `message` fields, `error` is dropped.
> 
> 
> [1] -
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/WebChannel.
> jsm#121
> [2] -
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> js#327

Right, I think this is because we never needed any "errors" to bubble up to the content server. Do we need to receiver errors now? If so, then we should add it.

Comment 2

[Shane Tomlinson [:stomlinson]]

The strange part here is a WebChannelMessageToContent event is sent to the content server with only an `id`, neither `message` nor `error` are present. So we receive a message, but can't really do anything with it.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #2)
> The strange part here is a WebChannelMessageToContent event is sent to the
> content server with only an `id`, neither `message` nor `error` are present.
> So we receive a message, but can't really do anything with it.

Right - that makes sense from the code - it tries to send |message| but that's effectively dropped as it's null/undefined.

As Vlad implies, I don't see any problem with always attempting to send |error| too, which will be dropped in the success case in the same way |message| is in the error case.  OTOH though, I'm not sure what errors are being sent this way that imply some action the server can take - IIUC, these error events tend to imply logic errors and that "real" errors the browser explicitly chooses to send would tend to be send as normal messages.

Comment 4

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #3)

> OTOH though, I'm not sure what errors are
> being sent this way that imply some action the server can take - IIUC, these
> error events tend to imply logic errors and that "real" errors the browser
> explicitly chooses to send would tend to be send as normal messages.

The useful one here would have been "No Such Channel". The only other error sent this way is "Message principal missing".

Would it make more sense to send these errors using the "message" field, the same as other errors. I imagine lines 127-130 would change to


targetBrowser.messageManager.sendAsyncMessage("WebChannelMessageToContent", {
  id: id,
  message: { error: errorMsg },
}, { eventTarget: eventTarget }, targetPrincipal);

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #4)
> (In reply to Mark Hammond [:markh] from comment #3)
> 
> > OTOH though, I'm not sure what errors are
> > being sent this way that imply some action the server can take - IIUC, these
> > error events tend to imply logic errors and that "real" errors the browser
> > explicitly chooses to send would tend to be send as normal messages.
> 
> The useful one here would have been "No Such Channel". The only other error
> sent this way is "Message principal missing".

I still don't understand what scenarios would cause this error to be generated and what the server would do on receipt.

eg, IIRC, "No Such Channel" would only realistically happen if somehow the channel wasn't created or was prematurely closed by the client code, and "Message principal missing" implies a client that's even more screwed up again, so I'm unclear on what mitigations the JS code should try and make in these cases.

IOW, given this implies a seriously screwed-up client I'm not sure how attempting to work around them is worthwhile.

> Would it make more sense to send these errors using the "message" field, the
> same as other errors. I imagine lines 127-130 would change to

I'm not sure what "other errors" you refer to here, but if you mean errors private to the channel (ie, that correctly functioning code at either end of the channel might choose to send) then I think they are a different category of errors.

Comment 6

[Shane Tomlinson [:stomlinson]]

(In reply to Mark Hammond [:markh] from comment #5)
> I still don't understand what scenarios would cause this error to be
> generated and what the server would do on receipt.
> 
> eg, IIRC, "No Such Channel" would only realistically happen if somehow the
> channel wasn't created

This is precisely the scenario we saw in https://github.com/mozilla/fxa-content-server/issues/2518, and it was a pain to debug. We received a WebChannelMessageToContent event with neither a message nor an error, only an id. A pretty unhelpful message. If we are going to send a WebChannelMessageToContent, it should at least be something with some information that is usable.

 or was prematurely closed by the client code, and
> "Message principal missing" implies a client that's even more screwed up
> again, so I'm unclear on what mitigations the JS code should try and make in
> these cases.
> 
> IOW, given this implies a seriously screwed-up client I'm not sure how
> attempting to work around them is worthwhile.

The client would not attempt to work around the error, rather the client developer would have information that is useful for debugging, e.g., on a "No such channel" error, the developer would at least know "I've either gotta set up that channel, or ask somebody else to do it for me."

> 
> > Would it make more sense to send these errors using the "message" field, the
> > same as other errors. I imagine lines 127-130 would change to
> 
> I'm not sure what "other errors" you refer to here, but if you mean errors
> private to the channel (ie, that correctly functioning code at either end of
> the channel might choose to send) then I think they are a different category
> of errors.

"other errors" meaning anything normally sent in message.error field. Perhaps you are right though, perhaps combining those errors would be like combining Network and Transport layer errors in the OSI networking model, an attempt to combine two different layers of the stack.

The "No such channel" error, at a minimum, should be surfaced to developers to give *some* information to go on.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Tomlinson [:stomlinson] from comment #6)
> This is precisely the scenario we saw in
> https://github.com/mozilla/fxa-content-server/issues/2518

Ah, ok, that makes perfect sense then :) 

Note I wasn't objecting to fixing things so the error made it back, I just wanted to make sure the reason you were getting the error in the first place doesn't point to a deeper problem. But earlier firefoxes not having the channel is the bit I was missing.

I'm still inclined to suggest we don't add error inside message though but instead make it "either message or data"

Comment 8

[Mark Banner (:standard8)]

I've come across this bug due from bug 1154251 where we're going to be using web channels for some of the Hello code. Given that gecko can send a message back if the WebChannel doesn't exist, that would be very useful for us, as we'll then be able to detect older versions, and can move onto the next thing, rather than having to timeout.

However, I can't see where in the code you're talking about changing to return the error message. I'm also a bit unclear by what you mean by either "message or data".

Can you enlighten me please?

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Banner (:standard8) from comment #8)

> However, I can't see where in the code you're talking about changing to
> return the error message. I'm also a bit unclear by what you mean by either
> "message or data".

That was a typo - I meant to say "message or error".

IIRC, the issue is that an error message is sent to the content via https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/WebChannel.jsm#121 - it constructs the data element with |id| and |error| attributes (whereas a non-error data element has |id| and |message| at line 276)

The code at https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-content.js#678 then receives this message and turns it into an event for the actual content to see - but it drops |error| - so we could just add a new line after the |message| attribute to read |error: e.data.error| with the expectation being that either message or error will be undefined.

OTOH though, that still leaves an unsatisfactory story for errors - eg, the literal string "No Such Channel" has suddenly become part of the contract with the remote content, which seems unfortunate. Do we *really* need to expose the error, or just the fact one occurred? If the application wants to add richer errors as part of |message| they are still free to do so - this case is just about internal errors that prevent the channel from working (ie, that prevent us getting |message| in the first place)

Comment 10

[Mark Banner (:standard8)]

(In reply to Mark Hammond [:markh] from comment #9)
> OTOH though, that still leaves an unsatisfactory story for errors - eg, the
> literal string "No Such Channel" has suddenly become part of the contract
> with the remote content, which seems unfortunate. Do we *really* need to
> expose the error, or just the fact one occurred?

For my use case, I think we just need to know that one occurred/the request wasn't handled. message == null doesn't seem to adequately cover this from an explicit point of view, though I agree we don't want to necessarily depend on strings. How about error: true, or something like that?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Banner (:standard8) from comment #10)
> How about error: true, or something like that?

SGTM.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Banner (:standard8) from comment #10)
> message == null doesn't seem to adequately cover this from
> an explicit point of view

Thinking a little more though, what would it then mean to find no |error| *and* no |message|? If you think of |message == null| as meaning "I was unable to get a message", then it sounds more palatable. I don't feel strongly either way, but the thought of adding a new state that we consider "impossible" seems less than ideal and something clients may be forced to consider when writing their handlers (ie, wouldn't their code be more solid if they treated that as the |error == true| case anyway?)

Comment 13

[Ryan Kelly [:rfkelly]]

It sounds like this was the same bug as Bug 1320189, so closing as a dupe.  Please re-open if that's not the case.