787517 - mozbrowser throws an exception when its iframe is removed from the DOM

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

You have two test cases already in the tree with the removal part commented out.

See:
dom/indexedDB/test/file_app_isolation.js
(actual tests are test_app_isolation_inproc.html and test_app_isolation_oop.html)
and
extensions/cookie/test/test_permissionmanager_app_isolation.html

Comment 1

[Andrew Overholt [:overholt]]

We discussed this in triage and decided this is a blocker.

Comment 2

[Justin Lebar (not reading bugmail)]

I agree.

Comment 3

[Justin Lebar (not reading bugmail)]

I'm not actively working on this, so unassigning myself.  Maybe Dale will get to it before me.

Comment 4

[Justin Lebar (not reading bugmail)]

I think this is a relatively simple fix, should be a good first bug.

Comment 5

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

Just to make sure if I reproduce this bug correctly: uncomment the line of removing iframe in file_app_isolation.js, then run test_app_isolation_oop.html. I get a failure result because an exception from BrowserElementParent.js. (It doesn't crash the whole program.)

Is anybody working on this bug? If not, may I take this bug?

Comment 6

[Justin Lebar (not reading bugmail)]

> I get a failure result because an exception from BrowserElementParent.js. (It doesn't crash the 
> whole program.)

I think that's correct.  I don't think there's a crash.

> Is anybody working on this bug? If not, may I take this bug?

Go for it!

Comment 7

[Andrew Overholt [:overholt]]

(In reply to Patrick Wang [:kk1fff] from comment #5)
> Is anybody working on this bug? If not, may I take this bug?

Thanks, Patrick, I've assigned the bug to you.

Comment 8

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

Attachment: Test case to reproduce this bug

Test case for removing mozbrowser in inproc and oop mode. This test case may reproduce this bug.

Comment 9

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

Attachment: Test case to reproduce this bug

Update bug number in comment of previous version.

Comment 10

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

Attachment: Patch: Catch exceptions in callback functions.

Catch NS_ERROR_NOT_INITIALIZED exceptions of:
(1) callback functions.
(2) BEP's 'mozvisibilitychange' handler.

For (2), should we just remove the listener after NS_ERROR_NOT_INITIALIZE is caught?

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 659662 [details] [diff] [review]
Test case to reproduce this bug

>diff --git a/dom/browser-element/mochitest/browserElement_RemoveBrowserElement.js b/dom/browser-element/mochitest/browserElement_RemoveBrowserElement.js
>+/* Any copyright is dedicated to the public domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+"use strict";

Please add a comment here with the relevant bug number and briefly explaining
what you're testing.

>+  window.error = function(e) {

Isn't it window.onerror?  (or window.addEventListener("error", ...).)  Maybe
this test works because mochitest is already listening to onerror and causes
the test to fail if it hears an error.

>+    receivedFail = true;
>+    throw e;
>+  };
>+}

r=me with these nits addressed.

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 659664 [details] [diff] [review]
Patch: Catch exceptions in callback functions.

> Catch NS_ERROR_NOT_INITIALIZED exceptions of:
> (1) callback functions.

Why aren't we catching the error elsewhere?  If I call _goBack on an iframe
that's not in the DOM, I'm likewise going to get an exception, which seems no
good.

I know we discussed this on IRC, but I forget the conclusion we reached.

The only place where I care if the sendAsyncMsg failed is in sendDOMRequest (I
think!), because (to fix bug 787519), I want to fire onerror there if the
request failed.  Everywhere else, it looks like we want to silently ignore the
error.  Unless I'm missing something?

If I'm not missing something, how about we make _sendAsyncMsg silently eat
NS_ERROR_NOT_INITIALIZED?  Then in bug 787519, we can make _sendAsyncMsg return
a boolean (success/failure) and make _sendDOMRequest do the right thing if
_sendAsyncMsg returns false.

> (2) BEP's 'mozvisibilitychange' handler.
> 
> For (2), should we just remove the listener after NS_ERROR_NOT_INITIALIZE is caught?

Probably not, since then we'd have to add it back if the iframe were
re-inserted into the DOM.

Comment 13

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

Attachment: Test case to reproduce this bug

Comment 14

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

Attachment: Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg()

Prevent NS_ERROR_NOT_INITIALIZED from being thrown outside sendAsyncMsg.

Comment 15

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

(In reply to Justin Lebar [:jlebar] from comment #12)
> how about we make _sendAsyncMsg silently eat
> NS_ERROR_NOT_INITIALIZED?  Then in bug 787519, we can make _sendAsyncMsg
> return
> a boolean (success/failure) and make _sendDOMRequest do the right thing if
> _sendAsyncMsg returns false.
> 
Okay. For bug 787519, I've reproduced it today, it is a little bit different from this one. BEP is notified 'oop-frameloader-crashed' before getScreenshot() called, and _sendAsyncMsg() doesn't throw in that bug. I think I can submit patch for that bug in days.

Comment 16

[Justin Lebar (not reading bugmail)]

> and _sendAsyncMsg() doesn't throw in that bug.

Ah, interesting.  Okay!

We probably should still fire onerror on the DOMRequest when we get an exception, because I presume  we /would/ get an exception if you did getScreenshot() on an iframe that's not in the DOM.  I'm happy if you do that in this bug or in a separate bug.

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 660032 [details] [diff] [review]
Test case to reproduce this bug

> +// Bug 787517: Remove iframe in the handler of showmodalprompt.

Nit: This is a good description of what to do to /cause/ the problem, but I'd also like you to describe what the problem we're testing for actually is.  For example:

"Remove the iframe from the DOM during the showmodalprompt handler.  This shouldn't cause an exception to be thrown."

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 660039 [details] [diff] [review]
Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg()

+      // Handle NS_ERROR_NOT_INITIALIZED
+      if (e.result == Cr.NS_ERROR_NOT_INITIALIZED) {
+        debug("Handle NS_ERROR_NOT_INITIALIZED");
+        return;
+      }

"Handle" isn't the right word; it's more like we're /ignoring/ NS_ERROR_NOT_INITIALIZED.

Please indicate in the comment why it's OK to ignore this particular error.  Perhaps  "Ignore NS_ERROR_NOT_INITIALIZED: It is thrown when we call sendAsyncMessage if our frame element is not in the DOM, in which case there's no child to receive the message."

Comment 19

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

Attachment: Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg()

r+ in comment 18. fix nit.

Comment 20

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

Attachment: Test case to reproduce this bug

r+ in comment 17. update comment.

Comment 21

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d59d22a34c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9374b6502e7a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d59d22a34c1
https://hg.mozilla.org/mozilla-central/rev/9374b6502e7a