Closed Bug 632835 Opened 9 years ago Closed 9 years ago

Firefox doesn't pass iframe test from bug 363109 correctly

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: michal, Assigned: smaug)

References

(Depends on 1 open bug)

Details

(Whiteboard: [has patch])

Attachments

(4 files, 2 obsolete files)

The iframe test behaves weird after 363109 landed. Some iframes are duplicated
and some are missing when pressing F5.
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}.
Assignee: michal.novotny → nobody
Component: Networking: HTTP → Document Navigation
QA Contact: networking.http → docshell
Attached patch fix (obsolete) — Splinter Review
Attachment #514359 - Flags: review?(bzbarsky)
Blocks: 588643, 462076
blocking2.0: --- → ?
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.
Assignee: nobody → michal.novotny
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Whiteboard: [hardblocker] → [hardblocker][has patch][needs review bz]
The bug is somewhere else since the assertion few lines above is triggered.
Or, hmm, the patch does fix the assertion case.
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;
> }
>
And if we take this, this must go to b12
(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.
As I said the patch does fix the assertion.
If you address my comment, I'll review immediately :)
I wonder how to test this.
(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.
But did you look at the reason for that code.
InsertObjectAt can automatically increase the array size by 1.
In fact, you should now change the comment which is talking about InsertObjectAt
(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.
(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.
This should go to betaN. Changes to session history are rather scary and regression risky.
blocking2.0: final+ → ?
Attached patch patch v2Splinter Review
Removed the SetCount() block, because ReplaceObjectAt() does the same as SetCount() when the array isn't big enough.
Attachment #514359 - Attachment is obsolete: true
Attachment #514474 - Flags: review?(Olli.Pettay)
Attachment #514359 - Flags: review?(bzbarsky)
Attachment #514474 - Flags: review?(Olli.Pettay) → review+
(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 on attachment 514474 [details] [diff] [review]
patch v2

Boris, please have a quick look at the patch too.
Attachment #514474 - Flags: review?(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?
Attachment #514474 - Flags: review?(bzbarsky) → review+
There is an assert few lines above.
Oh, the NS_WARN_IF_FALSE there?
Oh, oops, it is a warning nowadays
(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.
blocking2.0: ? → final+
Is this ready to land?  If so, I can probably do the deed tonight.
Whiteboard: [hardblocker][has patch][needs review bz] → [hardblocker][has patch]
...on tryserver. Can't reproduce locally.
Although if I load the test case separately, I do get first the red rectangle and then green - even without the patch.
Apparently it isn't failing always...investigating.
Michal told that the tryserver pushes were using an old tree. Waiting for new results.
http://hg.mozilla.org/mozilla-central/rev/6229fab0d2e6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I wondered why one of my local patches was rejected: Bug 609396
Bug 609396 and Bug 624917 closed.
(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
Backed out

http://hg.mozilla.org/mozilla-central/rev/151d55505c10
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 636776
Attached patch patch (obsolete) — Splinter Review
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
The problem with testing is that httpd.js can't really handle this kind of case :(
Assignee: michal.novotny → Olli.Pettay
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.
Attachment #515362 - Flags: review?(bzbarsky)
Comment on attachment 515362 [details] [diff] [review]
patch

this is still a bit wrong
Attachment #515362 - Flags: review?(bzbarsky)
Whiteboard: [hardblocker][has patch] → [hardblocker]
Blocks: 609396
Blocks: 624917
Attached patch patchSplinter Review
Attachment #515362 - Attachment is obsolete: true
Attachment #515384 - Flags: review?(bzbarsky)
Whiteboard: [hardblocker] → [hardblocker][has patch]
Depends on: 637132
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.
(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.
Well, I'd more like to understand why this was marked as hardblocker.
But anyway,  there is a patch.
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.
blocking2.0: final+ → -
Whiteboard: [hardblocker][has patch] → [has patch]
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.
Attachment #515384 - Flags: review?(bzbarsky) → review+
Blocks: 640486
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.
Attached patch +commentsSplinter Review
Attachment #518722 - Flags: review?(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.
Attachment #518722 - Flags: review?(bzbarsky) → review+
Attached patch patchSplinter Review
http://hg.mozilla.org/mozilla-central/rev/ee2e22296d3c
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Blocks: 589457
Does the bug 652034 is connected to this issue affecting iframe rendering?
(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?
Duplicate of this bug: 656352
You need to log in before you can comment on or make changes to this bug.