787519 - getScreenShot() does not fire onsuccess or onerror when attempting to get a screenshot from a crashed app

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

So the latest windows manager introduces making screenshots and use it to do opening/closing transition. It would naively call getScreenShot() to a frame even if it's crashed. The expected behavior here is for getScreenShot() to call onerror on the returned DOM request.

Comment 1

[Justin Lebar (not reading bugmail)]

I agree this is a bug, but is it hurting you in any way, or can I safely ignore this for a while?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Justin Lebar [:jlebar] from comment #1)
> I agree this is a bug, but is it hurting you in any way, or can I safely
> ignore this for a while?

This has been workaround with a timeout, so no hurry I guess.

https://github.com/mozilla-b2g/gaia/pull/4251

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Returning error when FrameLoader fail to find a target to send message to.

Summarize of this patch:

1. When mAsyncCallback return false, nsFrameMessageManager::DispatchAsyncMessageInternal() returns an error code to make sendAsyncMessage throw.

2. Check if there's child tab in the same process when SendAsyncMessageToChild() fails to find TabParent of remote browser. Pointer to tab parent of remote browser is set null when the child process is crashed.

3. Handle the exception in BrowserElementParent.

I am a little worried about if this patch affects the other part of Gecko. Because sendAsyncMessage currently doesn't care if the message can be sent successfully, maybe sendAsyncMessage is called somewhere without handling it's exception. Although the try build seems fine. https://tbpl.mozilla.org/?tree=Try&rev=a65edfc38593

Comment 4

[Justin Lebar (not reading bugmail)]

Okay, this is very good!

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>-  nsRefPtr<nsIRunnable> ev =
>-    new nsAsyncMessageToChild(static_cast<nsFrameLoader*>(aCallbackData),
>-                                    aMessage, aData);
>-  NS_DispatchToCurrentThread(ev);
>-  return true;
>+  if (static_cast<nsFrameLoader*>(aCallbackData)->mChildMessageManager) {
>+    nsRefPtr<nsIRunnable> ev =
>+      new nsAsyncMessageToChild(static_cast<nsFrameLoader*>(aCallbackData),
>+                                aMessage, aData);
>+    NS_DispatchToCurrentThread(ev);
>+    return true;
>+  }
>+
>+  // We don't have any target to send asynchronous message to.
>+  return false;
> }

There's a bit of a race condition here.  In the in-process case, if
mChildMessageManager becomes null after we enqueue the nsAsyncMessageToChild
runnable then we won't deliever the message and we won't throw an exception.

There's probably a race condition in the OOP case as well, if the child crashes
but we don't get FrameLoader::DestroyChild in time.

Anyway, I don't think this is something to worry about, mostly because it's not
clear to me that it's a fixable problem...

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>@@ -321,21 +321,22 @@ BrowserElementParent.prototype = {
>   _sendAsyncMsg: function(msg, data) {
>     try {
>       this._mm.sendAsyncMessage('browser-element-api:' + msg, data);
>     } catch (e) {
>       // Ignore NS_ERROR_NOT_INITIALIZED. This exception is thrown when
>       // we call _sendAsyncMsg if our frame is not in the DOM, in which case
>       // we don't have child to receive the message.
>       if (e.result == Cr.NS_ERROR_NOT_INITIALIZED) {
>-        debug("Handle NS_ERROR_NOT_INITIALIZED");
>-        return;
>+        debug("Ignore NS_ERROR_NOT_INITIALIZED");
>+      } else {
>+        return false;
>       }
>-      throw e;
>     }
>+    return true;
>   },

Don't we want to fire an error on the DOM request if we get
NS_ERROR_NOT_INITIALIZED?  And if so, you can add a test for that, right?

>@@ -454,21 +455,24 @@ BrowserElementParent.prototype = {
>-  _sendDOMRequest: function(msgName) {
>+  _sendDOMRequest: function(domName, msgName) {
>     let id = 'req_' + this._domRequestCounter++;
>     let req = Services.DOMRequest.createRequest(this._window);
>-    this._pendingDOMRequests[id] = req;
>-    this._sendAsyncMsg(msgName, {id: id});
>+    if (this._sendAsyncMsg(msgName, {id: id})) {
>+      this._pendingDOMRequests[id] = req;
>+    } else {
>+      Services.DOMRequest.fireErrorAsync(req, "Unable to send message for: " + domName);
>+    }
>     return req;
>   },

The error message here should be different; the fact that we're sending
messages isn't relevant to webpages.

I'm tempted to just send "failed" as the error message.

> I am a little worried about if this patch affects the other part of Gecko.
> Because sendAsyncMessage currently doesn't care if the message can be sent
> successfully, maybe sendAsyncMessage is called somewhere without handling
> it's exception.

Indeed.  smaug may be able to give us a better indication as to whether this is
safe.  I'd like him to look at the next version of your patch.

We should at least update the IDL in nsIMessageManager.idl for nsIMessageSender
to indicate that sendAsyncMessage can throw.

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Returning error when FrameLoader fail to find a target to send message to.

Summarize this update:
1. BEP._sendAsyncMsg() returns false when an exception is caught.
2. Update error message that passed to onerror handler.

This patch involves some modification of nsFrameLoader and makes mm throwing an exception when the message receiver cannot be found, this exception isn't thrown currently. Could :smaug helps to give me some feedback? Thanks.

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case

Test if DOMRequest returned by an iframe gets an error callback when the iframe is not in the DOM.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 662021 [details] [diff] [review]
Returning error when FrameLoader fail to find a target to send message to.

>diff --git a/content/base/public/nsIMessageManager.idl b/content/base/public/nsIMessageManager.idl
>--- a/content/base/public/nsIMessageManager.idl
>+++ b/content/base/public/nsIMessageManager.idl
>@@ -217,16 +217,19 @@ interface nsIMessageSender : nsIMessageL
> {
>   /**
>    * Send |messageName| and |obj| to the "other side" of this message
>    * manager.  This invokes listeners who registered for
>    * |messageName|.
>    *
>    * See nsIMessageListener::receiveMessage() for the format of the
>    * data delivered to listeners.
>+   * @throws NS_ERROR_NOT_INITIALIZED if the element is not in the DOM.
>+   * @throws NS_ERROR_FAILURE when message receiver cannot be found, might
>+   *         because of child process crashed.

nsIMessageSender is not necessarily associated with any DOM element, so we need
to change the first @throws here.  Perhaps something like

  @throws NS_ERROR_NOT_INITIALIZED if the sender is not initialized.  For
  example, we will throw NS_ERROR_NOT_INITIALIZED if we try to send a message
  to a cross-process frame but the other process has not yet been set up.

The second @throws is basically fine, but to nit on the English, we need to
clean up the part after the comma a bit.  Perhaps we could be parallel with the
@throws above and say

  @throws NS_ERROR_FAILURE when the message receiver cannot be found.  For
  example, we will throw NS_ERROR_FAILURE if we try to send a message to a
  cross-process frame whose process has crashed.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -2239,21 +2239,26 @@ bool SendAsyncMessageToChild(void* aCall
> 
>         blobParents.AppendElement(blobParent);
>       }
>     }
> 
>     return tabParent->SendAsyncMessage(nsString(aMessage), data);
>   }
> 
>-  nsRefPtr<nsIRunnable> ev =
>-    new nsAsyncMessageToChild(static_cast<nsFrameLoader*>(aCallbackData),
>-                                    aMessage, aData);
>-  NS_DispatchToCurrentThread(ev);
>-  return true;
>+  if (static_cast<nsFrameLoader*>(aCallbackData)->mChildMessageManager) {
>+    nsRefPtr<nsIRunnable> ev =
>+      new nsAsyncMessageToChild(static_cast<nsFrameLoader*>(aCallbackData),
>+                                aMessage, aData);
>+    NS_DispatchToCurrentThread(ev);
>+    return true;
>+  }
>+
>+  // We don't have any target to send asynchronous message to.

"any targets", not "any target", and "/an/ asynchronous message" (or "the
asynchronous message" or "our asynchronous message").

r=me with these nits addressed!

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case

r+ in previous version. Add bug number in message of the changeset.

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Returning error when FrameLoader fail to find a target to send message to.

Fix comments.

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Returning error when FrameLoader fail to find a target to send message to.

Fix comments. (One nit is missed in previous version)

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 662493 [details] [diff] [review]
Returning error when FrameLoader fail to find a target to send message to.

FYI, when you get r+ with comments, you don't need to mark the next patch as r? -- you can go right to checkin-needed.  Of course you're welcome to mark the patch as r? if there's something you'd like me to look at.

Comment 12

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68537c3ed046
https://hg.mozilla.org/integration/mozilla-inbound/rev/909bd3f7c66a

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/68537c3ed046
https://hg.mozilla.org/mozilla-central/rev/909bd3f7c66a

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Justin Lebar [:jlebar] from comment #11)
> Comment on attachment 662493 [details] [diff] [review]
> Returning error when FrameLoader fail to find a target to send message to.
> 
> FYI, when you get r+ with comments, you don't need to mark the next patch as
> r? -- you can go right to checkin-needed.  Of course you're welcome to mark
> the patch as r? if there's something you'd like me to look at.

Got it! I thought r=me means I should mark r? again. :)