345857 - Crash [@ nsSubDocumentFrame::Reflow] when clicking talkback on ynet.co.il with Adblock Plus installed

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Uri Bernstein]

Steps to reproduce:

1. Install Adblock Plus (I use v. 0.7.1.1): http://adblockplus.org/en/installation and restart Firefox to activate the extension. No need to create any filters.
2. Go to the URL listed above.
3. Scroll down to the talkback section (below the blue bar).
4. Click on the text next to the number "1".
5. Crash.

This regressed between 2006-07-20-08 and 2006-07-21-05.
This might have the same root cause as bug 336718, but the regression date is different, so I'm filing a separate bug.

I'll attach an Apple crash report in a minute. Talkback is not working for me with recent builds.

I can't make a local testcase for this (a locally-saved version of the page doesn't crash, but doesn't work either).

Comment 1

[Uri Bernstein]

Attachment: crash report

Comment 2

[Uri Bernstein]

Oops. I meant to submit this as UNCONFIRMED. Sorry.

Comment 3

[Niro]

Crashed also on windows XP (PC).

Comment 4

[Régis Caspar]

from mozillazine:
"dutchguy"
  I get a crash at http://www.apcstart.com when I have adblockplus on. Anyone know 
  if this is an adblock problem or a firefox problem?
"me"
  Got a crash [@ nsSubDocumentFrame::Reflow ] with adblock plus 0.7.1.1 too: 
  TB21482324X Minefield/3.0a1 ID:2006072604 [cairo]
"tosha42"
  WFM, Minefield 2006072604 and AdBlock Plus 0.7.1.1, with private filterset not 
  using filterset G.

Comment 5

[Eyal Rozenberg]

Seeing this as well. I have a TB for the crash but the #$^&* TB server is down again. SM trunk 2006-08-09 on WinXP 32-bit.

Comment 6

[Eyal Rozenberg]

If you filter away the all the i/frames, the problem does not manifest.

Comment 7

[Wladimir Palant]

Same bug seems to be striking on http://www.map24.com/. If you enter any search string the applet appears for a moment and then Firefox crashes. Stack is identical to the one given above. Talkbacks:

TB26058107Z
TB26057311Y
TB26057246W

Comment 8

[Wladimir Palant]

I looked through the check-ins (http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=CoreTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-20+04%3A00&maxdate=2006-07-21+09%3A00&cvsroot=%2Fcvsroot). There are three bugs that seem likely to be the cause of this but the fact that www.ynet.co.il uses embed tags probably rules out bug 343604. This leaves us with bug 344861 and bug 213701. Personally I suspect the former, maybe bz or jst could comment...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Well, I can reproduce this.  Looking.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Is there a bug database for adblock plus?  No matter what happens with this bug, some of the stuff they're doing is Not OK (like "makes it exploitable" not ok).

Comment 11

[Wladimir Palant]

There is: http://adblockplus.org/en/bugs

Comment 12

[Wladimir Palant]

I backed out the patch for bug 344861 and it no longer crashes, on neither of the two sites.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, yeah.  I've been going through it step by step for the last 40 minutes trying to figure out exactly why that causes a crash...

Comment 14

[Boris Zbarsky [:bzbarsky]]

So here's the sequence of events here, as best I can tell:

1)  The page sets innerHTML on a node.
2)  We parse the HTML and start appending the new nodes.
3)  We append an <iframe>.
4)  The <iframe>'s BindToTree starts the document load.
5)  Adblock Plus is called.
6)  Adblock Plus gets the contentWindow from the iframe and gets its .document.
7)  Getting the document triggers CreateAboutBlankContentViewer, since we have
    no document so have to create one.
8)  nsDocShell::SetupNewViewer calls GetPositionAndSize.
9)  We do a layout flush.
10) There's a pending restyle which we process.
11) We reconstruct a frame for an ancestor of the new <iframe>.  
12) Since the <iframe> node is already in the tree, we construct a frame for
    it.
13) We unwind back to step 2 and notify on the new <iframe>.

So we end up with two frames for the <iframe>, one of which never sets itself up properly because it assumes that having a fully-setup docshell means it's all set.  Then we try to reflow it and crash.

I suppose we could null-check mContentViewer in the docshell and not flush if it's null... but that feels like a terrible hack.  The real issue here, imo, is that random things are happening under BindToTree.  Maybe we should put off loading of subdocuments (and images, plugins, etc) until the outermost update batch ends?  Sicking, what do you think?

Comment 15

[Boris Zbarsky [:bzbarsky]]

The other "possibility" is to flush out all pending restyles before inserting nodes when inserting a document fragment.  But I'm not sure that would work if some of the nodes are stylesheets...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Though I should note that step 6 (Adblock Plus getting the .document) arguably violates rule #4 at <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/public/nsIContentPolicy.idl&rev=3.8#180>, since .document depends on the current state of the DOM inside the iframe in question.

But I'd rather eliminate those restrictions if we can...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Another possibility is to disable flushes (by setting a boolean on the document) while inside BindToTree.

Comment 18

[Jesse Ruderman]

nsSubDocumentFrame::Reflow is the #9 trunk topcrash.

Comment 20

[Gerd]

(In reply to comment #7)
> Same bug seems to be striking on http://www.map24.com/. If you enter any search
> string the applet appears for a moment and then Firefox crashes. 

It is not necessary to enter a search string. Just click "Search", that's enough :(

Comment 21

[Eli Friedman]

Attachment: Proof of concept

Just a little experiment I did.  It doesn't crash if the frame loader starts loading the document before BindToTree.

I'm not going to suggest this get checked in, though, because it likely has some bad side-effects; I think it leaks, although I'm not sure. It also has some interesting side effects that probably need more consideration, like allowing iframes to load content without actually being inserted into the document.

Probably the correct solution is to kick off loads asynchronously like nsObjectLoadingContent, so that we're not in the middle of parsing when we start the subdocument load.

Comment 22

[Boris Zbarsky [:bzbarsky]]

You can't start loading before BindToTree, because you don't know what you're supposed to load.  The base URI depends on the position in the tree.  All hail xml:base!

Comment 23

[Eli Friedman]

Oops... didn't realize that.  The code doesn't really make that clear at all.

I guess the comments like http://lxr.mozilla.org/seamonkey/source/content/base/src/nsObjectLoadingContent.cpp#1021
are wrong?

Should http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLObjectElement.cpp#225 be checking IsInDoc() instead of aNotify?  (I think this could cause us to load the wrong content in certain cases... although it isn't visible because we load the correct URL when the object is put into the document.)

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I guess the comments like

No, it's ok.  We don't want to drop the existing docshell on removal from document, really.  We just have to because docshell can't deal right now.

> Should
>htttp://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLObjectElement.cpp#225
>be checking IsInDoc() instead of aNotify?

Well, there's no reason not to load an image, say, into an object which is not in the document.  You just might have to change which image it is when you get put in the document.  See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsObjectLoadingContent.cpp&rev=1.37&mark=720-736#729

We'll want to make your change eventually, probably; first we have to teach docshell to survive outside an existing docshell hieararchy, and more importantly to deal with being inserted into one in mid-load.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Borderline for blocking1.9+, except it looks hard, so [wanted-1.9].

Comment 26

[Wladimir Palant]

Attachment: Minimal testcase

I got a report that the same crash happens if you search for anything on ebay.com. Managed to reverse-engineer what this "eBay DOM" code is doing and cut the testcase down to only a few lines. eBay could easily fix this by creating the iframe first and setting overflow:hidden afterwards instead of the other way around - but I think this is critical and should be blocking1.9+. Btw, talkback from my crash on ebay.com is TB30379357M.

Comment 27

[Wladimir Palant]

Attachment: Minimal content policy for testing

I got rid of requesting the document of the new frame too early but it didn't solve the problem. It seems that Adblock Plus doesn't even come to requesting the document - it is .contentWindow.top request that causes the crash. Attaching a minimal content policy that does nothing else, simply drop it into the components directory - no extensions required to reproduce the crash.

Comment 28

[Wladimir Palant]

Testing for a custom property on the content window (with XPCNativeWrappers) also crashes. This pretty much means that I cannot test whether it is a newly created frame or an old frame that changed it location - I cannot access the data I stored on the frame's window object. I don't think there is a work-around for that...

Comment 29

[Wladimir Palant]

I think I managed to work around this issue (still testing this huge hack but looks good so far). I will try to release Adblock Plus 0.7.3 within the next two weeks.

Comment 30

[Wayne Mery (:wsmwk)]

(In reply to comment #29)
> I think I managed to work around this issue (still testing this huge hack but
> looks good so far). I will try to release Adblock Plus 0.7.3 within the next
> two weeks.

Wladimir is the new version (0.7.5.1) just a work around?

Comment 31

[Mike Shaver (:shaver emeritus)]

Wladimir: can you confirm that this is FIXED, as I think I understand from comment 29?

Comment 32

[Wladimir Palant]

No, comment 29 was only about a work-around. However, the testcase doesn't seem to crash Minefield any more (build 2007111905) so this is FIXED.

Comment 33

[Uri Bernstein]

(In reply to comment #32)
> No, comment 29 was only about a work-around. However, the testcase doesn't seem
> to crash Minefield any more (build 2007111905) so this is FIXED.
> 

Unless we know exactly what fixed it, this is WORKSFORME, not FIXED.

Comment 34

[Wladimir Palant]

It was probably bug 336718 - but whatever.