632835 - Firefox doesn't pass iframe test from bug 363109 correctly

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Michal Novotny [:michal]]

The iframe test behaves weird after 363109 landed. Some iframes are duplicated
and some are missing when pressing F5.

Comment 1

[Michal Novotny [:michal]]

Although I was able to reproduce the problem only with the iframe test with unexpected body, this problem is completely unrelated to the changes in bugs 363109, 628832 and 632061. The wrong code is IMO in nsSHEntry::AddChild (http://hg.mozilla.org/mozilla-central/annotate/1da3405c74fd/docshell/shistory/src/nsSHEntry.cpp#l648). This code can't work in case we don't add children in sequence. E.g. when adding child2 at index 1 and then child1 at index 0 we will end up with {child1, NULL, child2} instead of {child1, child2}.

Comment 2

[Michal Novotny [:michal]]

Attachment: fix

Comment 3

[Michal Novotny [:michal]]

testcase - http://michal.etc.cz/bug632835/test.html

1) Clear the cache.
1) Load the page.
2) Reload the page - iframes are duplicated and misplaced.

Comment 4

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

The bug is somewhere else since the assertion few lines above is triggered.

Comment 5

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

Or, hmm, the patch does fix the assertion case.

Comment 6

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

Comment on attachment 514359 [details] [diff] [review]
fix

>-  if (aOffset > mChildren.Count()) {
>-    mChildren.SetCount(aOffset);
>+  if (aOffset+1 > mChildren.Count()) {
>+    mChildren.SetCount(aOffset+1);
>   }
Why this change?
(And nit, it should be aOffset + 1, not aOffset+1)


>-  if (!mChildren.InsertObjectAt(aChild, aOffset)) {
>+  if (!mChildren.ReplaceObjectAt(aChild, aOffset)) {
>     NS_WARNING("Adding a child failed!");
>     aChild->SetParent(nsnull);
>     return NS_ERROR_FAILURE;
>   }
> 
>   return NS_OK;
> }
>

Comment 7

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

And if we take this, this must go to b12

Comment 8

[Michal Novotny [:michal]]

(In reply to comment #4)
> The bug is somewhere else since the assertion few lines above is triggered.

If you mean the "NS_WARN_IF_FALSE(dyn ...)" then this could happen e.g. in following case:

Add children in sequence 1,0,2. Due to described problem there is already a child on position 2 (which should be at 1) when adding the last child and this triggers the warning.

Comment 9

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

As I said the patch does fix the assertion.

Comment 10

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

If you address my comment, I'll review immediately :)

Comment 11

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

I wonder how to test this.

Comment 12

[Michal Novotny [:michal]]

(In reply to comment #6)
> >-  if (aOffset > mChildren.Count()) {
> >-    mChildren.SetCount(aOffset);
> >+  if (aOffset+1 > mChildren.Count()) {
> >+    mChildren.SetCount(aOffset+1);
> >   }
> Why this change?
> (And nit, it should be aOffset + 1, not aOffset+1)

This is IMO correct, since the aOffset counts from 0. E.g. if we are replacing an object at index 4 we need to have an array of size 5 or more.

Comment 13

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

But did you look at the reason for that code.
InsertObjectAt can automatically increase the array size by 1.

Comment 14

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

In fact, you should now change the comment which is talking about InsertObjectAt

Comment 15

[Michal Novotny [:michal]]

(In reply to comment #13)
> But did you look at the reason for that code.
> InsertObjectAt can automatically increase the array size by 1.

But this isn't the case of ReplaceObjectAt, right?


(In reply to comment #14)
> In fact, you should now change the comment which is talking about
> InsertObjectAt

True, I overlooked this.

Comment 16

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

(In reply to comment #15)
> (In reply to comment #13)
> > But did you look at the reason for that code.
> > InsertObjectAt can automatically increase the array size by 1.
> 
> But this isn't the case of ReplaceObjectAt, right?
Yeah, actually ReplaceObjectAt can add any number new objects.
So that code could be just removed.

Comment 17

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

This should go to betaN. Changes to session history are rather scary and regression risky.

Comment 18

[Michal Novotny [:michal]]

Attachment: patch v2

Removed the SetCount() block, because ReplaceObjectAt() does the same as SetCount() when the array isn't big enough.

Comment 19

[Johnathan Nightingale [:johnath]]

(In reply to comment #17)
> This should go to betaN. Changes to session history are rather scary and
> regression risky.

Scary, I agree, but the kind of scary we need a beta for, or the kind that would be evident on nightlies?

Comment 20

[Michal Novotny [:michal]]

Comment on attachment 514474 [details] [diff] [review]
patch v2

Boris, please have a quick look at the patch too.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514474 [details] [diff] [review]
patch v2

This is probably fine, yes.  Can we add a testcase for this?

Also, do we have existing tests that do some iframe insertion/removal that might trigger this code replacing a non-null entry?

In fact, should we be asserting that the thing we're replacing is null?

Comment 22

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

There is an assert few lines above.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Oh, the NS_WARN_IF_FALSE there?

Comment 24

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

Oh, oops, it is a warning nowadays

Comment 25

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

(In reply to comment #19)
> Scary, I agree, but the kind of scary we need a beta for, or the kind that
> would be evident on nightlies?
Well, ok, nightlies might be enough, and there will be RC anyway.
Backing this out would be trivial.

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Is this ready to land?  If so, I can probably do the deed tonight.

Comment 27

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

Seems like the patch causes 
http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/reftest/plugin-alpha-zindex.html?force=1
to fail on linux.

Comment 28

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

...on tryserver. Can't reproduce locally.

Comment 29

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

Although if I load the test case separately, I do get first the red rectangle and then green - even without the patch.

Comment 30

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

Apparently it isn't failing always...investigating.

Comment 31

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

Michal told that the tryserver pushes were using an old tree. Waiting for new results.

Comment 32

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

http://hg.mozilla.org/mozilla-central/rev/6229fab0d2e6

Comment 33

[Stefan]

I wondered why one of my local patches was rejected: Bug 609396

Comment 34

[Stefan]

Bug 609396 and Bug 624917 closed.

Comment 35

[Michal Novotny [:michal]]

(In reply to comment #31)
> Michal told that the tryserver pushes were using an old tree. Waiting for new
> results.

Reftests are green with the latest source.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=e4103fd9494a

Comment 36

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

Backed out

http://hg.mozilla.org/mozilla-central/rev/151d55505c10

Comment 37

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

Attachment: patch

I should have realized that the simple ReplaceObjectAt change can't work.
I need to write testcases for this, but did upload to tryserver already.
http://hg.mozilla.org/try/rev/322c8912f909

Comment 38

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

The problem with testing is that httpd.js can't really handle this kind of case :(

Comment 39

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

Comment on attachment 515362 [details] [diff] [review]
patch

Boris, what do you think of this?
mPossibleDynamicChildIndex is there just to limit iterations in common cases.

Comment 40

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

Comment on attachment 515362 [details] [diff] [review]
patch

this is still a bit wrong

Comment 41

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

Attachment: patch

Comment 42

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

Btw, I'm not sure I understand why this is a hardblocker.
(Sure this is a regression.)

As far as I know, writing proper tests for this isn't possible with our current
httpd.js setup.

Comment 43

[(no longer active)]

(In reply to comment #42)
> Btw, I'm not sure I understand why this is a hardblocker.
> (Sure this is a regression.)

Then renom please so that this shows up in the triage sessions.

Comment 44

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

Well, I'd more like to understand why this was marked as hardblocker.
But anyway,  there is a patch.

Comment 45

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

This missed beta 12, so based on comment 7 minusing this.

Also, this seems somewhat high risk to me (which I guess is why olli wrote comment 7), so unless this is also high value, it doesn't seem like we should do this for FF4.

Does anyone know of real sites that this breaks? If so, feel free to renominate if you think the sites are high profile enough.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 515384 [details] [diff] [review]
patch

>@@ -628,38 +628,84 @@ nsSHEntry::AddChild(nsISHEntry * aChild,
>+    if (aOffset < mChildren.Count()) {
>+      for (PRInt32 i = aOffset; i < mChildren.Count(); ++i) {

The |if| there is redundant; the for loop checks that already.

>+    // If the new child isn't dynamically added, it should be set to aOffset.
>+    // If there are dynamically added children before that, those must be
>+    // moved to be after aOffset.
>+    PRInt32 end = PR_MIN(mChildren.Count(), aOffset + 1);
>+    for (PRInt32 i = 0; i < end; ++i) {

Wouldn't it make more sense to walk backwards from aOffset and stop if we find a non-dynamically-added entry?  Then we'd have to almost no work in the case of no dynamically added entries.

r=me with those fixed.

Comment 47

[Benjamin Smedberg]

This breaks http://www.carivit.it according to bug 640486, but the workaround is to reload the page once, which I think is acceptable for now.

Comment 48

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

Attachment: +comments

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 518722 [details] [diff] [review]
+comments

>+++ b/docshell/shistory/src/nsSHEntry.cpp
>+    PRInt32 start = PR_MIN(mChildren.Count() - 1, aOffset);

This is ok for now, but if mChildren becomes an nsTArray, you could run into trouble.  Consider the case when mChildren.Count() == 0 and aOffset is 1.  Will the signed/unsigned promotions work out correctly?  I dunno, and better to not have to worry about it.  Probably safer to condition this whole bit on mChildren.Count() > 0, since if it's 0 there's nothing to do here anyway.

r=me with that.

Comment 50

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

Attachment: patch

Comment 51

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

http://hg.mozilla.org/mozilla-central/rev/ee2e22296d3c

Comment 52

[Loic]

Does the bug 652034 is connected to this issue affecting iframe rendering?

Comment 53

[Mounir Lamouri (:mounir)]

(In reply to comment #52)
> Does the bug 652034 is connected to this issue affecting iframe rendering?

Loic, given that the bug has been fixed, could you check if bug 652034 is fixed in Aurora or Nightly?