Jesse Ruderman Reporter
	Description • 15 years ago

###!!! ASSERTION: Shouldn't happen here: '!IsTablePseudo(inFlowFrame)', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 8826

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 15 years ago

Oh, this is more fun due to bug 135040, and the assertion is quite correct we're ending up with the wrong primary frame for the content node!  The content model here before the removeChild call in boom2() is:

<map>
  <area/>
  <span>
    <img/>
    <td/>
  </span>
</map>

The frames look like this:

  Block(map)
    Inline(span)
      Image(img)
      TableOuter(span)
        Table(span)
          RowGroup(span)
            Row(span)
              Cell(td)

Now we make a GetPrimaryFrameFor() call on the <span>.  It's an inline, so not in the hashtable, so we look for the primary frame.  Its previous sibling in the DOM is the <area>, so we pass its primary frame (the Image, due to bug 135040) as the hint.mPrimaryFrameForPrevSibling to FindPrimaryFrameFor.  This calls FindFrameWithContent, and passes it that hint.  So we get the next sibling of the hint frame, and lo and behold: its content node is the one we want (it's the anonymous outer table in that frame tree above).  So we return it.

If I use GECKO_FRAMECTOR_DEBUG_FLAGS=fast-find-frame in my environment, we of course detect this issue immediately in FindPrimaryFrameFor:

  ##!! ASSERTION: hint shortcut found wrong frame: 'verifyTestFrame == *aFrame'

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 15 years ago

Oh, and this bug predates all the table changes; they just made the bug assert.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 15 years ago

I wanted to add some asserts to FindFrameWithContent, but can't think of a reasonable thing to assert...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 15 years ago

Jesse, does that test need to land as a crashtest, or is it already one of our crashtests?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 15 years ago

I guess it could land as a reftest too.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 15 years ago

On first load this works correctly, but on reload it doesn't make the image go away in Fx3...

I'd love to know how to make this into a reliable reftest!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 15 years ago

Maybe with preloading the image before doing all that DOM creation?  I suspect the key here is that it fails when the image doesn't have to reframe on load.

	Jesse Ruderman Reporter
	Comment 8 • 15 years ago

My testcase is not an existing automated test.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 15 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/683b00c3d1d1, which includes a reftest that fails reliably on both Fx3 and trunk without this patch.

I think we should take this on both branches; without this we end up with frames mapping content that's no longer in the document, which can get a little weird, and the patch is quite safe, in my opinion.

	David Baron :dbaron:
	Comment 11 • 15 years ago

Comment on attachment 371998 [details] [diff] [review]
As checked in; this applies to both branches after merging reftest.list

a1.9.1=dbaron

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 15 years ago

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8767a2c06da5

	Daniel Veditz [:dveditz]
	Comment 13 • 15 years ago

Comment on attachment 371998 [details] [diff] [review]
As checked in; this applies to both branches after merging reftest.list

Approved for 1.9.0.10, a=dveditz for release-drivers

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 15 years ago

Fixed in CVS.

	Al Billings [:abillings - ex-MoCo]
	Comment 15 • 15 years ago

Verified with attached testcase for 1.9.0.11 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11pre) Gecko/2009051305 GranParadiso/3.0.11pre (.NET CLR 3.5.30729).