1287717 - [Presentation WebAPI] PresentationConnectionClosedEvent is not fired on controller page when the MIME type of receiver page is not supported

Status: VERIFIED FIXED

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

Description

[Teri Wen [:twen]]

When the presentation URL is a pdf file, TV receives a blank page and the controller terminates connection. According to the W3C spec: https://www.w3.org/TR/presentation-api/#starting-a-presentation 
  21. If any of the following steps fails, abort all remaining steps and close the presentation connection S with error as closeReason, and a human readable message describing the failure as closeMessage.

The connection should be closed with error message.

STR:
1. Use the sample web page: https://people.mozilla.org/~schien/test-presentation.html
and change the presentation URL to a pdf file
2. Use Fennec to open the web page

Expected Behavior:
Show error with error message

Actual Behavior:
Show error message on controller, and possibly close presentation on receiver side

Comment 1

[Teri Wen [:twen]]

**Correction**
Actual Behavior:
Blank page on receiver, connection terminated on controller side

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Two things need to be done.

1. We will need to change the state transition from "terminated" to "closed" in controller side.
https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#769

2. We will need to close the receiver window if loading timeout is detected.
https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#1177

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: part 1, enter-closed-state-while-fail-to-connect.patch

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775025 [details] [diff] [review]
part 1, enter-closed-state-while-fail-to-connect.patch

ah, "closed means that the presentation connection has been closed, or could not be opened."

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: part 2, close-receiver-page-while-timeout.patch

For unreachable URL and unsupported MIME type, we can monitor the STATE_STOP since there is no STATE_TRANSFERRING.
@smaug, do you know if there is any way to test loading unreachable URL and unsupported MIME type in mochitest? I tried changing header with the ^header^ file but still get STATE_TRANSFERRING.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Didn't I just review something related to that... it was using information about the popup firefox has when unknown mime type is being loaded.
Yes, https://bugzilla.mozilla.org/show_bug.cgi?id=1282407.
Would that setup work here for testing?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

or perhaps other tests testing download attribute?

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: part 2, close-receiver-page-while-timeout.patch

found the correct way to use ^headers^ file, update patch with test cases.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775025 [details] [diff] [review]
part 1, enter-closed-state-while-fail-to-connect.patch

>   if (NS_WARN_IF(NS_FAILED(aReason) || !mIsResponderReady)) {
>     // The presentation session instance may already exist.
>     // Change the state to TERMINATED since it never succeeds.
>-    SetStateWithReason(nsIPresentationSessionListener::STATE_TERMINATED, aReason);
>+    if (nsIPresentationSessionListener::STATE_TERMINATED != mState) {
>+      SetStateWithReason(nsIPresentationSessionListener::STATE_CLOSED, aReason);
>+    }

Actually, the comment here is wrong, since it talks about "TERMINATED"

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09d52e94321b

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: part 1, enter-closed-state-while-fail-to-connect.patch

update according to comment #9, carry r+.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: pull request for tv 2.6

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): presentation api
User impact if declined: receiver page will not be closed when load failure
Testing completed: with mochitest
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a

Comment 13

[Carsten Book [:Tomcat]]

has problems to apply: renamed 1287717 -> close-receiver-page-while-timeout.patch
applying close-receiver-page-while-timeout.patch
patching file dom/presentation/tests/mochitest/mochitest.ini
Hunk #1 FAILED at 13
1 out of 2 hunks FAILED -- saving rejects to file dom/presentation/tests/mochitest/mochitest.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh close-receiver-page-while-timeout.patch

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: part 2, close-receiver-page-while-timeout.patch

resolve merge conflict, carry r+.

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Carsten Book [:Tomcat] from comment #13)
> has problems to apply: renamed 1287717 ->
> close-receiver-page-while-timeout.patch
> applying close-receiver-page-while-timeout.patch
> patching file dom/presentation/tests/mochitest/mochitest.ini
> Hunk #1 FAILED at 13
> 1 out of 2 hunks FAILED -- saving rejects to file
> dom/presentation/tests/mochitest/mochitest.ini.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh
> close-receiver-page-while-timeout.patch

resolved with new part 2 patch uploaded.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/025466d995ef
Part 1, enter closed state while fail to connect. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/734a7d13d285
Part 2, close receiver page while loading fail. r=smaug.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/025466d995ef
https://hg.mozilla.org/mozilla-central/rev/734a7d13d285

Comment 18

[Josh Cheng [:josh]]

Comment on attachment 8777243 [details] [review]
pull request for tv 2.6

Approve for TV 2.6

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8777243 [details] [review]
pull request for tv 2.6

ni? @xeonchen for uplift

Comment 20

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/1a310c179d7dece3b3c0225accfd6400742496ad

Comment 23

[Teri Wen [:twen]]

Hi SC,

Can you confirm the fixed behavior is as described in comment 2:
1. Close connection.
2. Close blank receiver window.

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Teri Wen [:twen] from comment #23)
> Hi SC,
> 
> Can you confirm the fixed behavior is as described in comment 2:
> 1. Close connection.
> 2. Close blank receiver window.

Yes, the behavior should be matched with your description. Did you see different behavior?

Comment 25

[Teri Wen [:twen]]

Everything looks right, just want to make sure I don't miss anything :)

Verified fixed.