[FlyWeb] Crash on Android when calling publishServer()

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Flyweb
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: justindarc, Assigned: justindarc)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

a year ago
Created attachment 8831831 [details] [diff] [review]
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!
Assignee: nobody → jdarcangelo
Status: NEW → ASSIGNED
Attachment #8831831 - Flags: review?(mconley)
Attachment #8831831 - Flags: feedback?(kvijayan)
Comment on attachment 8831831 [details] [diff] [review]
bug1335200.patch

Redirecting to daleharvey - if he can't review it, he'll probably know who can.
Attachment #8831831 - Flags: review?(mconley) → review?(dale)
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
Attachment #8831831 - Flags: review?(dale) → review?(s.kaspari)
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.
Attachment #8831831 - Flags: review?(s.kaspari) → feedback+
+CC jchen: This value showing up here could be caused by the refactoring of the messaging code that happened recently.
(Assignee)

Comment 6

a year ago
(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.
That was intentional. We only want to allow values listed in DoorHanger.Type, unless there's a reason not to?
(Assignee)

Comment 8

a year ago
(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?
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?
(Assignee)

Comment 10

a year ago
Created attachment 8832333 [details] [diff] [review]
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.
Attachment #8831831 - Attachment is obsolete: true
Attachment #8831831 - Flags: feedback?(kvijayan)
Attachment #8832333 - Flags: review?(s.kaspari)
(Assignee)

Comment 11

a year ago
Created attachment 8832334 [details] [diff] [review]
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.
Attachment #8832334 - Flags: review?(s.kaspari)
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!
Attachment #8832334 - Flags: review?(s.kaspari) → review+
Attachment #8832333 - Flags: review?(s.kaspari) → review-

Comment 13

a year ago
Pushed by justindarc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8bc6f7131a
[FlyWeb] Crash on Android when calling publishServer(), r=sebastian

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f8bc6f7131a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.