1644484 - Intermittent browser/base/content/test/fullscreen/browser_bug1620341.js | Uncaught exception - undefined - timed out after 50 tries.

Status: RESOLVED FIXED

(Firefox :: General, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: apavel [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=305655600&repo=mozilla-central
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/NnCb0-1ZRWCebYpQB3otSw/runs/0/artifacts/public/logs/live_backing.log

---

[task 2020-06-09T16:07:10.743Z] 16:07:10 INFO - TEST-START | browser/base/content/test/fullscreen/browser_bug1620341.js
[task 2020-06-09T16:07:12.620Z] 16:07:12 INFO - GECKO(2393) | JavaScript error: chrome://browser/content/browser-fullScreenAndPointerLock.js, line 533: TypeError: can't access property "osPid", childBC.currentWindowGlobal is null
[task 2020-06-09T16:07:12.620Z] 16:07:12 INFO - GECKO(2393) | JavaScript error: chrome://browser/content/browser-fullScreenAndPointerLock.js, line 557: InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable
[task 2020-06-09T16:07:17.742Z] 16:07:17 INFO - TEST-INFO | started process screentopng
[task 2020-06-09T16:07:18.120Z] 16:07:18 INFO - TEST-INFO | screentopng: exit 0
[task 2020-06-09T16:07:18.121Z] 16:07:18 INFO - Buffered messages logged at 16:07:10
[task 2020-06-09T16:07:18.121Z] 16:07:18 INFO - Entering test bound test_fullscreen_cross_origin
[task 2020-06-09T16:07:18.122Z] 16:07:18 INFO - Buffered messages logged at 16:07:11
[task 2020-06-09T16:07:18.122Z] 16:07:18 INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "https://example.com/browser/browser/base/content/test/fullscreen/fullscreen_frame.html" line: 0}]
[task 2020-06-09T16:07:18.123Z] 16:07:18 INFO - Buffered messages logged at 16:07:12
[task 2020-06-09T16:07:18.124Z] 16:07:18 INFO - Start fullscreen on iframe frameAllowed
[task 2020-06-09T16:07:18.124Z] 16:07:18 INFO - TEST-PASS | browser/base/content/test/fullscreen/browser_bug1620341.js | Request should be allowed - "fullscreenchange" == "fullscreenchange" -
[task 2020-06-09T16:07:18.125Z] 16:07:18 INFO - Console message: [JavaScript Warning: "Exited fullscreen because fullscreen element was removed from document." {file: "chrome://browser/content/tabbrowser.js" line: 3785}]
[task 2020-06-09T16:07:18.126Z] 16:07:18 INFO - Console message: [JavaScript Error: "TypeError: can't access property "osPid", childBC.currentWindowGlobal is null" {file: "chrome://browser/content/browser-fullScreenAndPointerLock.js" line: 533}]
[task 2020-06-09T16:07:18.128Z] 16:07:18 INFO - _sendMessageToTheRightContent@chrome://browser/content/browser-fullScreenAndPointerLock.js:533:22
[task 2020-06-09T16:07:18.128Z] 16:07:18 INFO - cleanupDomFullscreen@chrome://browser/content/browser-fullScreenAndPointerLock.js:490:15
[task 2020-06-09T16:07:18.129Z] 16:07:18 INFO - didDestroy@resource:///actors/DOMFullscreenParent.jsm:30:27
[task 2020-06-09T16:07:18.130Z] 16:07:18 INFO -
[task 2020-06-09T16:07:18.130Z] 16:07:18 INFO - Console message: [JavaScript Error: "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable" {file: "chrome://browser/content/browser-fullScreenAndPointerLock.js" line: 557}]
[task 2020-06-09T16:07:18.131Z] 16:07:18 INFO - _sendMessageToTheRightContent@chrome://browser/content/browser-fullScreenAndPointerLock.js:557:26
[task 2020-06-09T16:07:18.131Z] 16:07:18 INFO - cleanupDomFullscreen@chrome://browser/content/browser-fullScreenAndPointerLock.js:490:15
[task 2020-06-09T16:07:18.132Z] 16:07:18 INFO - didDestroy@resource:///actors/DOMFullscreenParent.jsm:30:27
[task 2020-06-09T16:07:18.132Z] 16:07:18 INFO -
[task 2020-06-09T16:07:18.133Z] 16:07:18 INFO - Buffered messages finished
[task 2020-06-09T16:07:18.133Z] 16:07:18 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/fullscreen/browser_bug1620341.js | Uncaught exception - undefined - timed out after 50 tries.
[task 2020-06-09T16:07:18.134Z] 16:07:18 INFO - Leaving test bound test_fullscreen_cross_origin
[task 2020-06-09T16:07:18.135Z] 16:07:18 INFO - GECKO(2393) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2020-06-09T16:07:18.136Z] 16:07:18 INFO - GECKO(2393) | MEMORY STAT | vsize 2838MB | residentFast 316MB | heapAllocated 115MB
[task 2020-06-09T16:07:18.137Z] 16:07:18 INFO - TEST-OK | browser/base/content/test/fullscreen/browser_bug1620341.js | took 7014ms
[task 2020-06-09T16:07:18.138Z] 16:07:18 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-06-09T16:07:18.139Z] 16:07:18 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/fullscreen/browser_bug1620341.js | Found an unexpected tab at the end of test run: data:text/html, <html xmlns="http://www.w3.org/1999/xhtml"> <head> <meta charset="utf-8"/> <title>First tab to be loaded</title> </head> <body> <button>JUST A BUTTON</button> </body> </html> -
[task 2020-06-09T16:07:18.141Z] 16:07:18 INFO - checking window state

Comment 1

[Intermittent Failures Robot]

10 failures in 5211 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 6
• autoland: 4

Platform breakdown:

• linux1804-64: 10

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1644484&startday=2020-06-08&endday=2020-06-14&tree=all

Comment 2

[Intermittent Failures Robot]

10 failures in 4035 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 7
• mozilla-central: 1
• try: 2

Platform breakdown:

• linux1804-64: 10

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1644484&startday=2020-06-15&endday=2020-06-21&tree=all

Comment 3

[:Gijs (he/him)]

I'm a bit surprised by this newly added test failing under fission pretty much from its introduction. https://bugzilla.mozilla.org/show_bug.cgi?id=1620341#c17 suggests that this failing only under fission may indicate a problem. Can you take a look?

Comment 4

[:Gijs (he/him)]

[task 2020-06-09T16:07:18.126Z] 16:07:18 INFO - Console message: [JavaScript Error: "TypeError: can't access property "osPid", childBC.currentWindowGlobal is null" {file: "chrome://browser/content/browser-fullScreenAndPointerLock.js" line: 533}]

and the other errors in comment #0 look like they may be related/helpful.

Comment 5

[Alphan Chen [:alchen]]

I checked the failure logs.
From them, we know sometimes the currentWindowGlobal under browsingContext is gone when DOMFullscreenParent.didDestroy() is called.

Maybe we can refine the "DOMFullscreenParent.hasBeenDestroyed()".
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/actors/DOMFullscreenParent.jsm#182
Now we only check if the browsingContext still exists.

Or we can add checks for "childBC.currentWindowGlobal" and "parentBC.currentWindowGlobal" and refine the logic of _sendMessageToTheRightContent().
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/base/content/browser-fullScreenAndPointerLock.js#534

What do you think?

Comment 6

[:Gijs (he/him)]

So my understanding is that https://searchfox.org/mozilla-central/rev/a87a1c3b543475276e6d57a7a80cb02f3e42b6ed/browser/base/content/tabbrowser.js#3697 removes the browser element, and this destroys the actor, at which point there is sometimes no browsing context, and sometimes a browsing context but no currentWindowGlobal anymore. Right?

I don't seem to really understand why this code works the way it does, though, and there are no comments explaining it... Both when entering ( https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/base/content/browser-fullScreenAndPointerLock.js#420-426 ) and leaving ( https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/base/content/browser-fullScreenAndPointerLock.js#490-492 ) we seem to make some kind of decision based on the return value of _sendMessageToTheRightContent - why? Is it just to avoid running the parent-process enter/exit code more than once? Or is there some other reason?

(In reply to Alphan Chen [:alchen] from comment #5)

I checked the failure logs.
From them, we know sometimes the currentWindowGlobal under browsingContext is gone when DOMFullscreenParent.didDestroy() is called.

Maybe we can refine the "DOMFullscreenParent.hasBeenDestroyed()".
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/actors/DOMFullscreenParent.jsm#182
Now we only check if the browsingContext still exists.

Can you elaborate on why this would help / what this would fix?

In particular, if the browser contains a same-origin subframe and we fullscreen from the inner frame, AIUI we would now return true from both the actor for the inner frame and the toplevel one, and both of them will try and do the rest of the "clean up fullscreen". That doesn't seem correct. Right?

Or we can add checks for "childBC.currentWindowGlobal" and "parentBC.currentWindowGlobal" and refine the logic of _sendMessageToTheRightContent().
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/base/content/browser-fullScreenAndPointerLock.js#534

What do you think?

Comment 7

[Intermittent Failures Robot]

15 failures in 5252 pushes (0.003 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 12
• mozilla-central: 3

Platform breakdown:

• linux1804-64: 15

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1644484&startday=2020-06-22&endday=2020-06-28&tree=all

Comment 8

[Alphan Chen [:alchen]]

Bug 1505916 comment 7 describes the whole flow of entering fullscreen mode.
Here are some documentation about how "_sendMessageToTheRightContent()" works.
https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/browser/base/content/browser-fullScreenAndPointerLock.js#408-419

After landing bug 1505916, there are two regressions about UI not properly restored when exiting fullscreen.

1. Bug 1588386 solves "this.browsingContext.top.embedderElement is gone" problem.
   https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/browser/actors/DOMFullscreenParent.jsm#43
2. After Bug 1588386, we add "hasBeenDestroyed()" to handle the case where the actor is already destroyed during fullscreen exit so that chrome UI is fully restored.

For the error of this bug,

1. it is called from DOMFullscreenParent:didDestroy()
2. the window is still with "inDOMFullscreen" attribute

For the first proposal, I just consider that this case also shows that the actor is destroyed.
Since it is called from "DOMFullscreenParent:didDestroy()", the actor should be destroyed.

Comment 9

[:Gijs (he/him)]

(In reply to Alphan Chen [:alchen] from comment #8)

Bug 1505916 comment 7 describes the whole flow of entering fullscreen mode.
Here are some documentation about how "_sendMessageToTheRightContent()" works.
https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/browser/base/content/browser-fullScreenAndPointerLock.js#408-419

I don't understand. The comment implies that _sendMessageToTheRightContent() is meant to notify more than 1 frame when run under Fission ("Additionally, ..."), but it only calls sendAsyncMessage either 0 or 1 time, so it's not clear how that happens.

It also doesn't answer my question:

we seem to make some kind of decision based on the return value of _sendMessageToTheRightContent - why? Is it just to avoid running the parent-process enter/exit code more than once?

In other words, what is the point/reason of the boolean return value of _sendMessageToTheRightContent ? Why do we need to distinguish where the message gets sent?

After landing bug 1505916, there are two regressions about UI not properly restored when exiting fullscreen.

1. Bug 1588386 solves "this.browsingContext.top.embedderElement is gone" problem.
   https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/browser/actors/DOMFullscreenParent.jsm#43
2. After Bug 1588386, we add "hasBeenDestroyed()" to handle the case where the actor is already destroyed during fullscreen exit so that chrome UI is fully restored.

For the error of this bug,

1. it is called from DOMFullscreenParent:didDestroy()
2. the window is still with "inDOMFullscreen" attribute

For the first proposal, I just consider that this case also shows that the actor is destroyed.
Since it is called from "DOMFullscreenParent:didDestroy()", the actor should be destroyed.

Sure, but if there was a tree with more than one nesting level of frames, AIUI there'd be multiple actors, and multiple actors will try to exit full screen. Maybe that's OK? It comes back to the earlier question: I don't understand what the "right" content thing is to which we're sending a message, and what the significance of the return value of the method is supposed to be, so it's hard to understand whether it's OK to not care about whether the actor being destroyed is in fact the actor "responsible" for the browser being in full screen.

Comment 10

[Alphan Chen [:alchen]]

(In reply to :Gijs (he/him) from comment #9)

I don't understand. The comment implies that _sendMessageToTheRightContent() is meant to notify more than 1 frame when run under Fission ("Additionally, ..."), but it only calls sendAsyncMessage either 0 or 1 time, so it's not clear how that happens.

It also doesn't answer my question:

In other words, what is the point/reason of the boolean return value of _sendMessageToTheRightContent ? Why do we need to distinguish where the message gets sent?

From my understanding, _sendMessageToTheRightContent(aActor, aMessage) is doing:

1. Search the first ancestor that lives in a different process.
   2-1 If we cannot find ancestor, return true.
   2-2 If there is ancestor, send the message to the ancestor and return false.
2. There are two messages: "DOMFullscreen:Entered" and "DOMFullscreen:CleanUp"
   3.1 When receiving "DOMFullscreen:Entered", it will call "windowUtils.remoteFrameFullscreenChanged(remoteFrame)" and set "this.isNotRequestSource" as true
   https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/browser/actors/DOMFullscreenChild.jsm#26
   Then "Document::RequestFullscreen()" -> "Document::RequestFullscreenInContentProcess()". -> "Document::ApplyFullscreen()" -> dispatch "MozDOMFullscreen:Entered" event -> @DOMFullscreenParent.jsm call window.Fullscreen.enterDomFullscreen -> go back to step 1
   3.2 When receiving "DOMFullscreen:CleanUp", it will call "windowUtils.exitFullscreen()" and set "this.isNotRequestSource" as true.
   https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/browser/actors/DOMFullscreenChild.jsm#53
   Then "Document::ExitFullscreenInDocTree(doc)" -> ExifFullscreenScriptRunnable() -> dispatch "MozDOMFullscreen:Exited" event -> @DOMFullscreenParent.jsm call window.Fullscreen.cleanupDomFullscreen -> go back to step 1

The function makes sure that we should notify all ancestors (from bottom to top) first.
If we cannot find an ancestor anymore, we can finish the rest of "Fullscreen.enterDomFullscreen" or "Fullscreen.cleanupDomFullscreen()".

(In reply to Alphan Chen [:alchen] from comment #8)

2. After Bug 1588386, we add "hasBeenDestroyed()" to handle the case where the actor is already destroyed during fullscreen exit so that chrome UI is fully restored.

This is bug 1590138. Add here for reference.

Comment 11

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #5)

Or we can add checks for "childBC.currentWindowGlobal" and "parentBC.currentWindowGlobal" and refine the logic of _sendMessageToTheRightContent().
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/base/content/browser-fullScreenAndPointerLock.js#534

Since proposal 1 might be too aggressive, I think we should try to find an ancestor if possible.
If "parentBC.currentWindowGlobal" exists, we should notify parentActor in this case.

Comment 12

[Alphan Chen [:alchen]]

Attachment: Bug 1644484 - Handle the TypeError and InvalidStateError when calling FullScreen.cleanupDomFullscreen() from DOMFullscreenParent.didDestroy() r=Gijs

Comment 13

[Alphan Chen [:alchen]]

I can reproduce the problem by running test verification locally.
I always see "childBC.currentWindowGlobal" is null and the following error at the same time.
They are from two different DOMFullscreenParent.didDestroy().

[JavaScript Error: "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable" {file: "chrome://browser/content/browser-fullScreenAndPointerLock.js" line: 557}]

The error is from aActor.requestOrigin.sendAsyncMessage().
https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/browser/base/content/browser-fullScreenAndPointerLock.js#557

In attachment 9160403 [details], I add a check for "aActor.requestOrigin.hasBeenDestroyed()" to prevent sending messages to a destroyed actor.
With the patch, I cannot reproduce the symptom anymore.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aff172a1f772
Handle the TypeError and InvalidStateError when calling FullScreen.cleanupDomFullscreen() from DOMFullscreenParent.didDestroy() r=Gijs

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/aff172a1f772

Comment 16

[Intermittent Failures Robot]

8 failures in 4516 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 7
• mozilla-central: 1

Platform breakdown:

• linux1804-64: 8

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1644484&startday=2020-06-29&endday=2020-07-05&tree=all