Removing AREA element makes the image disappear

RESOLVED FIXED

Status

()

Core
Layout
--
major
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: René v. Sweeden, Assigned: dholbert)

Tracking

(4 keywords)

1.8 Branch
regression, testcase, verified1.8.1.10, verified1.8.1.9
Points:
---
Bug Flags:
blocking1.8.1.9 +
blocking1.8.1.10 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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

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

Comment 2

10 years ago
Created attachment 285537 [details]
testcase
(Reporter)

Updated

10 years ago
Attachment #285537 - Attachment description: Textcase → When pressing the link it schould delete óne element inside the map element but it delete's all the element and the map element too.

Comment 3

10 years ago
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

10 years ago
I can reproduce the bug in Firefox 2.0.0.8, but not in Firefox 2.0.0.7.
Status: UNCONFIRMED → NEW
Component: General → Layout
Ever confirmed: true
Flags: blocking1.8.1.9?
Keywords: regression, testcase
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → layout
Hardware: PC → All
Summary: removeChild deletes all elements when assuming child → Removing AREA element makes the entire image map disappear
Version: unspecified → 1.8 Branch

Comment 5

10 years ago
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.
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?
Attachment #285537 - Attachment description: When pressing the link it schould delete óne element inside the map element but it delete's all the element and the map element too. → testcase
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?
Blocks: 382376
Flags: in-testsuite?
(Assignee)

Comment 8

10 years ago
(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).
Assignee: nobody → dholbert
Flags: blocking1.8.1.9?
(Assignee)

Comment 9

10 years ago
Created attachment 285768 [details]
testcase 2 (reduced)
(Assignee)

Updated

10 years ago
Attachment #285768 - Attachment is patch: false
(Assignee)

Updated

10 years ago
Attachment #285768 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 10

10 years ago
Created attachment 285781 [details] [diff] [review]
reftest (as patch)

Updated

10 years ago
Flags: blocking1.8.1.9? → blocking1.8.1.9+
Flags: blocking1.8.1.10? → blocking1.8.1.10+
(Assignee)

Comment 11

10 years ago
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)
Status: NEW → ASSIGNED
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...
(Assignee)

Comment 13

10 years ago
Created attachment 285791 [details]
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.
(Assignee)

Comment 14

10 years ago
Created attachment 285801 [details]
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.
(Assignee)

Comment 15

10 years ago
Created attachment 285829 [details] [diff] [review]
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.
Whiteboard: have patch, need review
(Assignee)

Comment 16

10 years ago
Created attachment 285916 [details] [diff] [review]
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
Attachment #285829 - Attachment is obsolete: true
(Assignee)

Comment 17

10 years ago
Comment on attachment 285916 [details] [diff] [review]
patch v2

oops, fixing one thing and reposting
Attachment #285916 - Attachment is obsolete: true
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.
(Assignee)

Comment 19

10 years ago
Created attachment 285919 [details] [diff] [review]
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) {
(Assignee)

Updated

10 years ago
Attachment #285919 - Flags: superreview?(dbaron)
Attachment #285919 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 years ago
Summary: Removing AREA element makes the entire image map disappear → Removing AREA element makes the image disappear
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?
(Assignee)

Comment 21

10 years ago
(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 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)
Attachment #285919 - Flags: superreview?(dbaron) → superreview+
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.
Attachment #285919 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 24

10 years ago
(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)
(Assignee)

Comment 25

10 years ago
Created attachment 285929 [details] [diff] [review]
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)
Attachment #285929 - Flags: approval1.8.1.9?
Attachment #285929 - Flags: approval1.8.1.10?
(Assignee)

Comment 26

10 years ago
Created attachment 285930 [details] [diff] [review]
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)
Attachment #285781 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #285930 - Flags: review?(bzbarsky)
(Assignee)

Comment 27

10 years ago
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
(Assignee)

Comment 28

10 years ago
(In reply to comment #27)
> Reftest checked in:
(to trunk)
Attachment #285930 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

10 years ago
Flags: in-testsuite? → in-testsuite+
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
Attachment #285929 - Flags: approval1.8.1.9?
Attachment #285929 - Flags: approval1.8.1.9+
Attachment #285929 - Flags: approval1.8.1.10?
Attachment #285929 - Flags: approval1.8.1.10+
(Assignee)

Comment 30

10 years ago
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
Keywords: fixed1.8.1.10, fixed1.8.1.9
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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 ? 
Keywords: fixed1.8.1.9 → verified1.8.1.9

Comment 32

10 years ago
The removeChild call shouldn't throw.  That sounds bad.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

10 years ago
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.
(Assignee)

Updated

10 years ago
Whiteboard: have patch, need review

Comment 34

10 years ago
Oh, it's expected to throw on the second and subsequent clicks.
changing back to fixed because of comment #34, thanks jesse
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 401055
Flags: blocking1.8.1.11+ → blocking1.8.1.10+
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
Keywords: fixed1.8.1.10 → verified1.8.1.10
You need to log in before you can comment on or make changes to this bug.