400421 - Removing AREA element makes the image disappear

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[René v. Sweeden]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8

When i use removeChild( child ) to delete the Child al the other elements just disapear without doing anything else as you can see on my example.

Reproducible: Always

Steps to Reproduce:
1. Creating <map>
2. Creating <area>'s
3. Deleting one <area>
4. All <area>'s disapear
Actual Results:  
The <area>'s disapear from the screen but are still useable in the HTML/JavaScript. But the schape's created with the <area> elements are gone and not useable on screen.

Expected Results:  
Only delete óne schape assigned as child and still show the other shape's on the screen.

Comment 1

[Jesse Ruderman]

Can you attach a simple testcase (HTML with script) that demonstrates the bug?

Comment 2

[René v. Sweeden]

Attachment: testcase

Comment 3

[Jesse Ruderman]

It works correctly for me with Firefox trunk on Mac.  When I click the link, only the top-left area becomes unclickable and untabbable.

Btw, "1" isn't a valid ID.  See http://www.w3.org/TR/html401/types.html#type-name.

Comment 4

[Jesse Ruderman]

I can reproduce the bug in Firefox 2.0.0.8, but not in Firefox 2.0.0.7.

Comment 5

[Erryn Pollock]

I just encountered the same issue with 2.0.0.8, but don't recall running into it with 2.0.0.7. This problem is why I came here, in fact.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Regression range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&date=explicit&mindate=2007-09-18+02%3A00&maxdate=2007-09-19+07%3A00

Points to bug 382376. The checkin comment also mentions bug 375299, should that bug have a fixed1.8.1.8 keyword?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Huh.  I have no idea why this would be a problem, especially on branch and not on trunk.  Daniel, do you have time to step through this and see what the <map> impl ends up seeing in terms of notifications and what it does with them?

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #7)
> Daniel, do you have time to step through this and see what the <map>
> impl ends up seeing in terms of notifications and what it does with them?

Yeah, I'll take a look at it tomorrow (Monday).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: reftest (as patch)

Comment 11

[Daniel Holbert [:dholbert]]

This is the list of document observers, in order, when we call nsDocument::ContentRemoved:
 0  nsBindingManager
 1  PresShell
 2  nsContentList
 3  nsImageMap

The issue here is probably that, now that we're using a forward-iterator on this list (after bug 382376's patch), we hit PresShell::ContentRemoved sooner, and that call actually **removes** the nsImageMap from the document observers list, before we've ever gotten to call nsImageMap::ContentRemoved.  So, nsImageMap::ContentRemoved never gets called.

On the other hand, when we did reverse-order iteration (before the patch for bug 382376), we'd call ContentRemoved on the nsImageMap first, since it's the final document-observer in the list.  So, while it still may end up getting removed from the document observers, we'd at least call ContentRemoved on it first.

I'm guessing that this is the root problem here -- we're removing the nsImageMap from the document observers list earlier, and so we never get to call nsImageMap::ContentRemoved.

The nsImageMap gets removed from the document's observer list within this hierarchy of calls:

 PresShell::ContentRemoved
 nsCSSFrameConstructor::ContentRemoved
 nsFrameManager::RemoveFrame
 ...
(haven't drilled down below this yet, will fill this out more in a later comment)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, because the primary frame for the <area> is the image, so we think we're removing the imageframe.  Right.

We probably need to have nsImageMaps be observers of the presshell, not of the document, and have it notify them before messing with the frame tree in this case.

On trunk we should work on not having the bogus primary frame mapping, of course...

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: backtrace for nsImageMap being removed from doc observer list

> I'm guessing that this is the root problem here -- we're removing the
> nsImageMap from the document observers list earlier, 

Here's the backtrace for where we take the nsImageMap out of the document observer list.  It's due to the nsImageFrame getting destroyed, and the nsImageMap getting destroyed as a result.  See previous comment about why it's getting destroyed earlier now.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: backtrace for removing primary frame mapping

(In reply to comment #12)
> Ah, because the primary frame for the <area> is the image, so we think we're
> removing the imageframe.  Right.

Ah, right.  This attached stacktrace from the (working) old reverse-iterator version shows where we used to remove the mapping from the area element to the image frame.  (within nsImageMap::ContentRemoved, as expected)  

That removal prevented the PresShell from later finding (and destroying) the nsImageFrame, within PresShell::ContentRemoved.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: first-pass patch

(In reply to comment #12)
> We probably need to have nsImageMaps be observers of the presshell, not of the
> document, and have it notify them before messing with the frame tree in this
> case.

Here's a first-pass patch implementing this strategy.  It fixes the testcase.  Haven't run it through reftests yet.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: patch v2

Overview of patch:

  - Added a document-observer list to the PresShell class, with AddObserver / RemoveObserver functions (in the nsIPresShell_MOZILLA_1_8_BRANCH2 interface -- let me know if we need to add a BRANCH3.)

  - Make ImageMaps add/remove themselves to the PresShell's observer list (instead of the Document's)

  - Have PresShell pass document-observer notifications onto its own observer list, for all the observer functions that nsImageMap implements. (AttributeChanged, ContentAppended, ContentInserted, ContentRemoved) [1]

[1] Maybe it should pass document-observer notifications on for *all* nsIDocumentObserver functions?   I imagine that's not necessary and may be wasteful, since I doubt we need anything aside from imageMaps to observe the presShell.  I added a XXXdholbert comment about this in nsPresShell.cpp

Comment 17

[Daniel Holbert [:dholbert]]

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

oops, fixing one thing and reposting

Comment 18

[Daniel Veditz [:dveditz]]

nsIPresShell_MOZILLA_1_8_BRANCH2 was added in 1.8.1.8 which we're trying to
kill off ASAP with 1.8.1.9, and it's a non-scriptable internal interface.
re-using _BRANCH2 doesn't seem like it would cause problems.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: patch v3

Previous patch used an incorrectly substituted version of nsIDocumentObservers::Contains() inside the new PresShell::AddObserver.

New patch makes this replacement:
- if (!mObservers.IndexOf(aObserver) != -1) {
+ if (mObservers.IndexOf(aObserver) == -1) {

Comment 20

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

Comment on attachment 285919 [details] [diff] [review]
patch v3

> class nsIPresShell_MOZILLA_1_8_BRANCH2 : public nsIPresShell_MOZILLA_1_8_BRANCH
> {

These should at least have a comment above them like "Added in Gecko 1.8.0.9.  For internal use only."

>+  virtual void AddObserver(nsIDocumentObserver* aObserver) = 0;
>+  virtual PRBool RemoveObserver(nsIDocumentObserver* aObserver) = 0;


I don't follow why this was a problem on branch but not on trunk.  Could you explain?

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #20)
> (From update of attachment 285919 [details] [diff] [review])
> > class nsIPresShell_MOZILLA_1_8_BRANCH2 : public nsIPresShell_MOZILLA_1_8_BRANCH
> > {
> 
> These should at least have a comment above them like "Added in Gecko 1.8.0.9. 
> For internal use only."
> 
> >+  virtual void AddObserver(nsIDocumentObserver* aObserver) = 0;
> >+  virtual PRBool RemoveObserver(nsIDocumentObserver* aObserver) = 0;
> 
> 
> I don't follow why this was a problem on branch but not on trunk.  Could you
> explain?
> 

The branch patch and the trunk patch for bug 382376 did different things.
 -- Trunk patch: Just moved observer notification from beginning to end of nsBindingManager::ContentRemoved

 -- Branch patch: **Reversed the order** of observer iteration in nsDocument::ContentRemoved, which caused this bug's problem.  (by notifying the PresShell before the ImageMap)

Comment 22

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

Comment on attachment 285919 [details] [diff] [review]
patch v3

sr=dbaron with the comment comment above fixed; it seems like we might want to take this on trunk as well, at least until we fix the bogus primary frame issue (may want to comment in that bug)

Comment 23

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

Comment on attachment 285919 [details] [diff] [review]
patch v3

Marking r=dbaron as well; bz said on IRC he doesn't feel the need to review separately.

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to comment #21)

To be more specific -- 
  In branch, the ImageMap and PresShell were both observers on the document.
  In trunk, the ImageMap and BindingManager are both listeners on the nsNodeUtils object, and the PresShell observes the BindingManager.

So, if we were to reverse the iteration order across nsNodeUtils' observers, it's possible we could recreate this bug on trunk.  But we didn't, and I'm guessing it's unlikely that we would (though maybe we should add a comment in nsNodeUtils on Trunk to this effect)

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: patch v3a (just added dbaron's suggested comment)

same as the posted r+sr'd patch, with this comment added:

+  // Added in Gecko 1.8.1.9. For internal use only.

above the new function declarations in nsIPresShell.h
(requested by dbaron in his review comments)

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: reftest (as patch) v2

Same as previously posted reftest, except I removed the useless "usemap" attribute on the IMG tag for the reference case.  (which doesn't have any map tags at all)

Comment 27

[Daniel Holbert [:dholbert]]

Reftest checked in:

RCS file: /cvsroot/mozilla/layout/reftests/bugs/400421-1.html,v
done
Checking in 400421-1.html;
/cvsroot/mozilla/layout/reftests/bugs/400421-1.html,v  <--  400421-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/400421-1-ref.html,v
done
Checking in 400421-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/400421-1-ref.html,v  <--  400421-1-ref.html
initial revision: 1.1
done
Checking in reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.176; previous revision: 1.175
done

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to comment #27)
> Reftest checked in:
(to trunk)

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 285929 [details] [diff] [review]
patch v3a (just added dbaron's suggested comment)

approved for 1.8.1.9 and 1.8.1.10, a=dveditz

To fix this for 1.8.1.9 please land on the GECKO181_20071004_RELBRANCH. 1.8.1.10 uses the normal mozilla 1.8 branch

Comment 30

[Daniel Holbert [:dholbert]]

Checked in to GECKO181_20071004_RELBRANCH:
------------------------------------------
Checking in layout/base/nsIPresShell.h;
/cvsroot/mozilla/layout/base/nsIPresShell.h,v  <--  nsIPresShell.h
new revision: 3.158.4.7.2.1; previous revision: 3.158.4.7
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.852.2.22.2.2; previous revision: 3.852.2.22.2.1
done
Checking in layout/generic/nsImageMap.cpp;
/cvsroot/mozilla/layout/generic/nsImageMap.cpp,v  <--  nsImageMap.cpp
new revision: 3.111.4.4.14.1; previous revision: 3.111.4.4
done



Checked in to MOZILLA_1_8_BRANCH:
---------------------------------
Checking in layout/base/nsIPresShell.h;
/cvsroot/mozilla/layout/base/nsIPresShell.h,v  <--  nsIPresShell.h
new revision: 3.158.4.8; previous revision: 3.158.4.7
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.852.2.24; previous revision: 3.852.2.23
done
Checking in layout/generic/nsImageMap.cpp;
/cvsroot/mozilla/layout/generic/nsImageMap.cpp,v  <--  nsImageMap.cpp
new revision: 3.111.4.5; previous revision: 3.111.4.4
done

Comment 31

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.9 using :
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.9pre)
Gecko/2007102404 BonEcho/2.0.0.9pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.9pre)
Gecko/2007102403 BonEcho/2.0.0.9pre

and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.9pre) Gecko/2007102404 BonEcho/2.0.0.9pre

the test cases pass for me, adding verified keyword.

One Note: Daniel (Holbert) when i use the reduced test case or the original testcase i get a Error Message in the Error Console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMHTMLMapElement.removeChild]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=285768 :: del :: line 5"  data: no]

thats expected or ? 

Comment 32

[Jesse Ruderman]

The removeChild call shouldn't throw.  That sounds bad.

Comment 33

[Daniel Holbert [:dholbert]]

I saw that too -- it only happened on the second or third click, when you're removing an area element that's already been removed.

I'll investigate and see when that exception started appearing.

Comment 34

[Jesse Ruderman]

Oh, it's expected to throw on the second and subsequent clicks.

Comment 35

[Carsten Book [:Tomcat]]

changing back to fixed because of comment #34, thanks jesse

Comment 37

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.10 using the testcases and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.10pre) Gecko/2007111404 BonEcho/2.0.0.10pre

- Adding verified keyword