1335200 - [FlyWeb] Crash on Android when calling publishServer()

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Flyweb, defect)

Description

[Justin D'Arcangelo [:justindarc]]

Moving discussion for the crash in Firefox for Android from here: https://bugzilla.mozilla.org/show_bug.cgi?id=1333909#c8

Comment 1

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1335200.patch

Mike, I wasn't sure who to have review this small patch to Fennec. This appears to stem from a recent regression where we weren't catching an IllegalArgumentException in Java. If you aren't the right person to review, could you please re-assign? Thanks!

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8831831 [details] [diff] [review]
bug1335200.patch

Redirecting to daleharvey - if he can't review it, he'll probably know who can.

Comment 3

[Dale Harvey (:daleharvey)]

Comment on attachment 8831831 [details] [diff] [review]
bug1335200.patch

Happy to look at this but since I am not a peer for this code its probably best I forward the review on to Sebastian

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8831831 [details] [diff] [review]
bug1335200.patch

Review of attachment 8831831 [details] [diff] [review]:
-----------------------------------------------------------------

This catches the error but we shouldn't be in this situation in the first place.

* Either the value of "category" shouldn't be FLYWEBPUBLISHSERVER (flyweb-publish-server) here
* Or if this is a different type then it should be added to DoorHanger.Type (and probably needs more code handling this type).

Can you see why this value ends up here in this place? Let me know if you need help debugging this.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

+CC jchen: This value showing up here could be caused by the refactoring of the messaging code that happened recently.

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> * Either the value of "category" shouldn't be FLYWEBPUBLISHSERVER
> (flyweb-publish-server) here
> * Or if this is a different type then it should be added to DoorHanger.Type
> (and probably needs more code handling this type).
> 
> Can you see why this value ends up here in this place? Let me know if you
> need help debugging this.

Sure. I was going to simply add FLYWEBPUBLISHSERVER to DoorHanger.Type to fix this, but when I looked into it closer, I noticed that the recent refactoring was not correctly catching the exception for "unknown" door hanger types. I'll update the patch to add the type as well as catching the exception.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

That was intentional. We only want to allow values listed in DoorHanger.Type, unless there's a reason not to?

Comment 8

[Justin D'Arcangelo [:justindarc]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> That was intentional. We only want to allow values listed in
> DoorHanger.Type, unless there's a reason not to?

Shouldn't it default to DoorHanger.Type.DEFAULT instead of crashing?

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

The old behavior is to ignore invalid types. The new behavior is to crash instead. You can make arguments for either way, but IMO passing in an invalid type indicates a bug that should be fixed, and crashing is a good way to notice the bug.

BTW, if you don't need extra functionality, can't you pass in "DEFAULT" as the door hanger type?

Comment 10

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1335200.patch

sebastian: Here's the updated patch that adds the new enum type. I chose to do this instead of reusing DEFAULT since its very likely that there will be some custom behavior that will be necessary to implement for it in the future.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Attachment: bug1335200.patch (with exception)

Here's an alternate version of the patch that crashes when encountering DoorHanger types. Just let me know which one we should land.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8832334 [details] [diff] [review]
bug1335200.patch (with exception)

Review of attachment 8832334 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should land this one: Adding a new type on JS side and not making it known on the Java side is a potential error so I'd like to keep the crash so that we can see those errors. Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8bc6f7131a
[FlyWeb] Crash on Android when calling publishServer(), r=sebastian

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f8bc6f7131a