Last Comment Bug 787517 - mozbrowser throws an exception when its iframe is removed from the DOM
: mozbrowser throws an exception when its iframe is removed from the DOM
Status: RESOLVED FIXED
[mentor=jlebar][lang=js][WebAPI:P3]
: feature
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Patrick Wang (Chih-Kai Wang) (:kk1fff)
:
Mentors:
Depends on:
Blocks: browser-api
  Show dependency treegraph
 
Reported: 2012-08-31 11:47 PDT by Mounir Lamouri (:mounir)
Modified: 2013-04-04 13:53 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Test case to reproduce this bug (4.65 KB, patch)
2012-09-10 03:22 PDT, Patrick Wang (Chih-Kai Wang) (:kk1fff)
no flags Details | Diff | Splinter Review
Test case to reproduce this bug (4.63 KB, patch)
2012-09-10 03:31 PDT, Patrick Wang (Chih-Kai Wang) (:kk1fff)
justin.lebar+bug: review+
Details | Diff | Splinter Review
Patch: Catch exceptions in callback functions. (4.30 KB, patch)
2012-09-10 03:40 PDT, Patrick Wang (Chih-Kai Wang) (:kk1fff)
no flags Details | Diff | Splinter Review
Test case to reproduce this bug (4.57 KB, patch)
2012-09-11 04:27 PDT, Patrick Wang (Chih-Kai Wang) (:kk1fff)
justin.lebar+bug: review+
Details | Diff | Splinter Review
Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg() (2.13 KB, patch)
2012-09-11 05:10 PDT, Patrick Wang (Chih-Kai Wang) (:kk1fff)
justin.lebar+bug: review+
Details | Diff | Splinter Review
Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg() (2.29 KB, patch)
2012-09-12 00:10 PDT, Patrick Wang (Chih-Kai Wang) (:kk1fff)
kk1fff: review+
Details | Diff | Splinter Review
Test case to reproduce this bug (4.62 KB, patch)
2012-09-12 00:12 PDT, Patrick Wang (Chih-Kai Wang) (:kk1fff)
kk1fff: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-08-31 11:47:42 PDT
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] 2012-09-05 10:51:29 PDT
We discussed this in triage and decided this is a blocker.
Comment 2 Justin Lebar (not reading bugmail) 2012-09-05 11:01:36 PDT
I agree.
Comment 3 Justin Lebar (not reading bugmail) 2012-09-05 11:40:44 PDT
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) 2012-09-05 11:45:22 PDT
I think this is a relatively simple fix, should be a good first bug.
Comment 5 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2012-09-06 02:50:00 PDT
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) 2012-09-06 06:07:21 PDT
> 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] 2012-09-06 12:59:00 PDT
(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) 2012-09-10 03:22:16 PDT
Created attachment 659661 [details] [diff] [review]
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) 2012-09-10 03:31:23 PDT
Created attachment 659662 [details] [diff] [review]
Test case to reproduce this bug

Update bug number in comment of previous version.
Comment 10 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2012-09-10 03:40:00 PDT
Created attachment 659664 [details] [diff] [review]
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) 2012-09-10 11:54:27 PDT
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) 2012-09-10 12:03:32 PDT
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) 2012-09-11 04:27:05 PDT
Created attachment 660032 [details] [diff] [review]
Test case to reproduce this bug
Comment 14 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2012-09-11 05:10:03 PDT
Created attachment 660039 [details] [diff] [review]
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) 2012-09-11 06:18:43 PDT
(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) 2012-09-11 07:23:04 PDT
> 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) 2012-09-11 07:28:18 PDT
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) 2012-09-11 07:31:01 PDT
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) 2012-09-12 00:10:46 PDT
Created attachment 660337 [details] [diff] [review]
Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg()

r+ in comment 18. fix nit.
Comment 20 Patrick Wang (Chih-Kai Wang) (:kk1fff) 2012-09-12 00:12:32 PDT
Created attachment 660338 [details] [diff] [review]
Test case to reproduce this bug

r+ in comment 17. update comment.

Note You need to log in before you can comment on or make changes to this bug.