Closed Bug 1192270 Opened 9 years ago Closed 9 years ago

Attribute mutation is fired after adding and removing the same attribute with mergeAttributeRecords: true set in Mutation Observer

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: simon.lindholm10, Assigned: smaug)

References

Details

When executing:

> document.body.setAttribute("a", "b");
> document.body.removeAttribute("a");

the markup view throws an exception at markup-view.js:2741 about calling a function on a null value. This is apparently because with |mergeAttributeRecords: true| the inspector actor now receives a record {attributeName: "a", newValue: null} about a non-existent attribute, which it does not expect.

The fix for this in devtools is trivial:

> [at inspector.js:816, updateMutation]
> -      if (!found) {
> +      if (!found && change.newValue !== null) {

but it's not clear to me whether this should be fixed there or in Gecko. Thoughts?
well, MutationObserver doesn't play with new values, so the result is expected.
I don't really see how else MO could do merging without loosing information.


Well, hmm, if old value is null and the new value is also new, that would mean an attribute is added and then removed. But we don't store the old value unless attributeOldValue is used for observing .
Assignee: nobody → bugs
Where should I execute 
document.body.setAttribute("a", "b");
document.body.removeAttribute("a");
to see the exception?
Open the Inspector, then press escape to open the split console. Running it from there should work (and log an exception to the browser console).
The bug seems to have morphed into showing `a="null"` in the Inspector view, rather than failing outright (same behavior in Nightly and Beta). Can we land the one liner mentioned in comment 0?
(In reply to Simon Lindholm from comment #4)
> The bug seems to have morphed into showing `a="null"` in the Inspector view,
> rather than failing outright (same behavior in Nightly and Beta). Can we
> land the one liner mentioned in comment 0?

Yeah, I can go ahead and fix this in the devtools server, at least as a work around.  I can file a new bug for that so we can refer back to this one to track any platform fix.  Olli, this is a bug in the MutationObserver with mergeAttributeRecords=true, right?
Flags: needinfo?(bugs)
I'd say no. 
As I said MutationObserver doesn't play with new values and merging just means we tell what the old value was (if old value is observed), and in case of adding attribute it is null. But then merging with attribute removal just keeps that information. But having the record for attribute change doesn't say anything about the existence of the attribute.

But I don't understand why we actually get "null" anywhere.
I see
2821         mutation.newValue = targetNode.hasAttribute(mutation.attributeName) ?
2822                             targetNode.getAttribute(mutation.attributeName)
2823                             : null;
So that should deal with attribute removals, including the case when there was a merge.
(In reply to Olli Pettay [:smaug] from comment #6)
> I'd say no. 
> As I said MutationObserver doesn't play with new values and merging just
> means we tell what the old value was (if old value is observed), and in case
> of adding attribute it is null. But then merging with attribute removal just
> keeps that information. But having the record for attribute change doesn't
> say anything about the existence of the attribute.
> 
> But I don't understand why we actually get "null" anywhere.
> I see
> 2821         mutation.newValue =
> targetNode.hasAttribute(mutation.attributeName) ?
> 2822                            
> targetNode.getAttribute(mutation.attributeName)
> 2823                             : null;
> So that should deal with attribute removals, including the case when there
> was a merge.

Yeah, I think the machinery handles this case in general, but this particular sequence of events didn't used to happen before merging started.  The fix in Bug 1209342 is handling the case where there is a 'new' attribute in the mutation that has a null value (the condition never happened before since we would get two records in this case).  I'll close this bug and finish up the work in Bug 1209342.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(bugs)
Resolution: --- → WONTFIX
Summary: [markup-view] Exception when adding and immediately removing attribute → Attribute mutation is fired after adding and removing the same attribute with mergeAttributeRecords: true set in Mutation Observer
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.