The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mounir, Assigned: kk1fff)

Tracking

(Blocks: 1 bug, {feature})

Trunk
mozilla18
feature
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [mentor=jlebar][lang=js][WebAPI:P3])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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
We discussed this in triage and decided this is a blocker.
blocking-basecamp: ? → +
I agree.
Blocks: 693515
I'm not actively working on this, so unassigning myself.  Maybe Dale will get to it before me.
Assignee: justin.lebar+bug → nobody
I think this is a relatively simple fix, should be a good first bug.
Whiteboard: [mentor=jlebar][lang=js]
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?
> 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!
Summary: Crash in mozbrowser when removing the iframe from the DOM → mozbrowser throws an exception when its iframe is removed from the DOM
Whiteboard: [mentor=jlebar][lang=js] → [mentor=jlebar][lang=js][WebAPI:P3]
(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.
Assignee: nobody → pwang
Keywords: feature
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.
Attachment #659661 - Flags: review?(justin.lebar+bug)
Created attachment 659662 [details] [diff] [review]
Test case to reproduce this bug

Update bug number in comment of previous version.
Attachment #659661 - Attachment is obsolete: true
Attachment #659661 - Flags: review?(justin.lebar+bug)
Attachment #659662 - Flags: review?(justin.lebar+bug)
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?
Attachment #659664 - Flags: feedback?(justin.lebar+bug)
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.
Attachment #659662 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #659664 - Flags: feedback?(justin.lebar+bug)
Created attachment 660032 [details] [diff] [review]
Test case to reproduce this bug
Attachment #659662 - Attachment is obsolete: true
Attachment #660032 - Flags: review?(justin.lebar+bug)
Created attachment 660039 [details] [diff] [review]
Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg()

Prevent NS_ERROR_NOT_INITIALIZED from being thrown outside sendAsyncMsg.
Attachment #659664 - Attachment is obsolete: true
Attachment #660039 - Flags: review?(justin.lebar+bug)
(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.
> 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 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."
Attachment #660032 - Flags: review?(justin.lebar+bug) → review+
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."
Attachment #660039 - Flags: review?(justin.lebar+bug) → review+
Created attachment 660337 [details] [diff] [review]
Patch: Catch NS_ERROR_NOT_INITIALIZED in _sendAsyncMsg()

r+ in comment 18. fix nit.
Attachment #660039 - Attachment is obsolete: true
Attachment #660337 - Flags: review+
Created attachment 660338 [details] [diff] [review]
Test case to reproduce this bug

r+ in comment 17. update comment.
Attachment #660032 - Attachment is obsolete: true
Attachment #660338 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d59d22a34c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9374b6502e7a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d59d22a34c1
https://hg.mozilla.org/mozilla-central/rev/9374b6502e7a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.