402982 - Unbinding an iframe can execute script via unload

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Unbinding an iframe destroys its frameloader, which fires unload on the document inside.  That's bad.

Comment 1

[Jesse Ruderman]

How bad?

Comment 2

[Boris Zbarsky [:bzbarsky]]

"call virtual functions on deleted objects" bad, if one tries hard enough.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Bug 395609 should fix this one too.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So one important question is, when do other browsers fire that event?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Could we do something as simple as make the destruction of the frameloader happen off an event?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> Could we do something as simple as make the destruction of the frameloader
> happen off an event?
That would probably break http://mozilla.pettay.fi/moztests/frame/iframe.html
Unload might happen after adding the element back to document.
The testcase doesn't seem to work with Opera, never get any unloads to main page.

Perhaps I'm too nervous about changing when unload event gets fired, but 
changing anything in load and unload events is regression-risky.

Will test Windows-Safari...

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Safari works like current Gecko, load->unload->element_removed ...
Could write some more complex testcases though.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Webkit seems to nowadays have "willRemoveChild" phase, and 
frameownerelement calls there
frame->disconnectOwnerElement();
frame->loader()->frameDetached();

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(Webkit's frame handling code looks quite terrible.)
"queuePostAttachCallback", which is used when attaching <frame>,
is somewhat similar to reflowcallback or documentbatch. 
'Do things when it is safe'

Comment 10

[Boris Zbarsky [:bzbarsky]]

One issue is that docshells don't exist well outside a docshell tree right now.  So we really want to destroy the frameloader ASAP after unbinding.

One option, in fact, is to do it right after the unbind call in nsGenericElement::doRemoveChildAt or some such.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I modified your testcase a bit and when unload fires, the node has already been removed from the document. So, how about we use something like your queuing solution to defer frameloader teardown from UnbindToTree to happen in the following EndUpdate instead? That should preserve current behaviour.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

That should work.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK. That should be fairly simple, I guess, you can hold a strong reference to the content element until we're ready to tear down the frameloader. How about you implement that and I'll work on bug 395609 so that XUL will follow the same path?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Ok.

Comment 15

[Boris Zbarsky [:bzbarsky]]

The EndUpdate thing is fine as long as we're not too worried about the behavior of script inside the update itself (mutation events) or script triggered by that EndUpdate.

Put another way, if an EndUpdate script (<script> element, XBL event handler, etc) reinserts the node in the document, should we destroy the frame loader?  Or even more simply, if we have two frames that are kids of a div, the div is removed from the document, the onload event of the first subframe puts the second subframe in the document.  Or into a different document.  Should the second frameloader get destroyed?  Or does the subframe's onload not fire in this case?  Perhaps it shouldn't (though the onload of the parent document still should, I guess).

I hate DOM events.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the EndUpdate callback would just check if the element is in the document, and destroy the frameloader if it isn't.

Comment 17

[Boris Zbarsky [:bzbarsky]]

As long as we destroy or queue for destruction any existing frameloader upon insertion into a document (where we currently create a frameloader), that would work, yes...

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess we need to queue frameloaders then, not elements.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Safe frame element docshell destroy

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good to me, but where did that r+ come from?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 298927 [details] [diff] [review]
Safe frame element docshell destroy

oops, I was going to ask review

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

And, bz, 

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

... if you're still busy, which I think, who could review this?
Jonas?

Comment 24

[Boris Zbarsky [:bzbarsky]]

It looks OK at first skim, but you're right: I'm not sure I'll have time before freeze to check in detail that it's happy.  Either sicking or jst should be able to review it, if they have time...

Please leave the XXX comment you're removing, though: this patch changes nothing about that situation.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 298927 [details] [diff] [review]
Safe frame element docshell destroy

Oh, I thought the XXX comment was just about this bug.

Comment 26

[Boris Zbarsky [:bzbarsky]]

No, the XXX comment is about being able to move the node around in the DOM without blowing away the docshell.  Little things like tag drag and drop between windows, say.  ;)

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Um, I think this should be P1.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 298927 [details] [diff] [review]
Safe frame element docshell destroy

+class nsAsyncFrameLoaderFinalize : public nsRunnable

Given that this class really just destroys the docshell, it would be more appropriately named if it was named nsAsyncDocShellDestroyer.

- In nsFrameLoader::Destroy()

+  if ((mInDestructor || !doc ||
+       NS_FAILED(doc->FinalizeFrameLoader(this))) && mDocShell) {
+    nsCOMPtr<nsIRunnable> event = new nsAsyncFrameLoaderFinalize(mDocShell);
+    NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
+    NS_DispatchToCurrentThread(event);
   }
-
-  mDocShell = nsnull;

In the case where we asynchronously destroy the docshell we also need to null out mDocShell since no other code does in that case.

r=jst with that changed. Given the time I'll attach a new patch that does that shortly to get this in tonight...

Comment 29

[Johnny Stenback (:jst)]

Attachment: Updated diff per above comment.

Comment 30

[Johnny Stenback (:jst)]

Moving this out to beta4 as we won't have enough time to understand this in enough detail tonight to land it.

Comment 31

[Johnny Stenback (:jst)]

Note also that since the IID of nsIDocument changes, we now need to update the IID in content/base/src/contentbase.gqi in addition to updating it in the header file.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

> Moving this out to beta4 as we won't have enough time to understand this in
> enough detail tonight to land it.
But we really should get this to beta4

> Note also that since the IID of nsIDocument changes, we now need to update the
> IID in content/base/src/contentbase.gqi in addition to updating it in the
> header file.
This is really one of the main reason why I don't like .gqi files at all.
Those pseudo-iids should be generated automatically.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 300293 [details] [diff] [review]
Updated diff per above comment.

I do think this could be done simpler once bug 401155 has landed as we wouldn't need any of the methods on nsIDocument. But we could just land for now and simplify later too.

- nsDocument.h

>+  nsTArray<nsRefPtr<nsFrameLoader>> mFinalizableFrameLoaders;

You'll need a space between the '>' to get this to compile everywhere. Otherwise it'll be parsed as a bit-shift operator and fail to compile on some compilers.

- nsDocument.cpp

>@@ -2662,6 +2664,15 @@ nsDocument::EndUpdate(nsUpdateType aUpdateType)
>   if (mScriptLoader) {
>     mScriptLoader->RemoveExecuteBlocker();
>   }
>+
>+  PRUint32 length = mFinalizableFrameLoaders.Length();
>+  if (length > 0) {
>+    nsTArray<nsRefPtr<nsFrameLoader> > loaders;
>+    mFinalizableFrameLoaders.SwapElements(loaders);
>+    for (PRInt32 i = 0; i < length; ++i) {
>+      loaders[i]->Finalize();
>+    }
>+  }

You only want to do this on the last EndUpdate, no?

- nsFrameLoader.cpp

>+class nsAsyncDocShellDestroyer : public nsRunnable
>+{
>+public:
>+  nsAsyncDocShellDestroyer(nsIDocShell* aDocShell)
>+    : mDocShell(aDocShell)
>+  {
>+  }
>+
>+  NS_IMETHOD Run()
>+  {
>+    nsCOMPtr<nsIBaseWindow> base_win(do_QueryInterface(mDocShell));
>+    if (base_win) {
>+      base_win->Destroy();
>+    }
>+  }
>+  nsRefPtr<nsIDocShell> mDocShell;
>+};

Couldn't this be done as an nsRunnableMethod<nsIBaseWindow>?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

> You'll need a space between the '>' to get this to compile everywhere.
> Otherwise it'll be parsed as a bit-shift operator and fail to compile on some
> compilers.
Ah, right. This wouldn't compile on Windows.

> You only want to do this on the last EndUpdate, no?
Yes. Where did that code disappear. I'm pretty sure I had it in some patch :/
I'll add if (mUpdateNestLevel == 0) around that code

> Couldn't this be done as an nsRunnableMethod<nsIBaseWindow>?
Ah, perhaps. I've got too used to nsRunnable classes.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

 > > Couldn't this be done as an nsRunnableMethod<nsIBaseWindow>?
And the answer is no. That requires that the method returns void.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Attachment: to be checked in

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Checked in with a bustage fix

Comment 38

[Justin Dolske [:Dolske]]

This seems to have caused problems with Adblock Plus. Backing out this patch (plus bustage fix) fixes things. [The other backout I did was 395609, but that didn't seem to help by itself, at least.]

I get gobs of console errors in my current trunk build:

this.boxObject is null" {file: "chrome://adblockplus/content/sidebar.js" line: 711}]' when calling method: [nsIContentPolicy::shouldLoad]"

Adblock doesn't seem to be blocking ads now, and it doesn't have a toolbar button.

Not sure exactly what Adblock is doing here; the code is in an implementation of nsITreeView::addItem().

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: backout patch for testing

Comment 40

[Justin Dolske [:Dolske]]

(To FIXED again, the regression in bug 419798 seems to have gone away and is likely due to a different problem).

Comment 41

[Daniel Veditz [:dveditz]]

Is this needed for the 1.8 branch?

Is there a testcase QA could use to check the branch and verify this has indeed been fixed on the trunk?

Comment 42

[Jeff Walden [:Waldo]]

Better yet, an automated test that runs on every checkin and can be run in 1.8 by just opening the test URL in a 1.8 build while a 1.9 Mochitest run happens?

Comment 43

[Boris Zbarsky [:bzbarsky]]

Please make the loop index in nsDocument::EndUpdate unsigned.  The code as written warns.

Comment 44

[Boris Zbarsky [:bzbarsky]]

I checked in a fix for comment 43; it was driving me nuts.