780199 - MutationObserver changes the behavior when setting <xul:browser>'s src to the same value it has

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[YUKI "Piro" Hiroshi]

Background:
"collapsed" attribute just hides any XUL elements even if it is inline frame (<browser/>). And, after the "collapsed" attribute is removed, the element becomes visible again. It is expected that "collapsed" attribute doesn't break the content of the inline frame. This is the most important point of this bug.

Problem:
When any MutationObserver instance becomes active in the chrome document, the behaviour described above becomes broken.

Environment:
Mozilla/5.0 (Windows NT 6.0; rv:14.0) Gecko/20100101 Firefox/14.0.1
Mozilla/5.0 (Windows NT 6.0; rv:17.0) Gecko/17.0 Firefox/17.0

Steps to reproduce:
 1. Start the Scratchpad.
    ("Tools" => "Web Developer" => "Scratchpad")
 2. Choose "Environment" => "Browser".
 3. Copy following codes, paste them to the scratchpad, and run it.
    -----------------------------------------------
    var sidebarBox = document.getElementById("sidebar-box");
    sidebarBox.hidden = false;
    var sidebarSplitter = document.getElementById("sidebar-splitter");
    sidebarSplitter.hidden = false;
    eval('window.toggleSidebar='+window.toggleSidebar.toString()
    .replace(/\.hidden/g, '.collapsed')
    .replace(/sidebar\.setAttribute\("src", "about:blank"\)/, '')
    .replace(/sidebar\.docShell\.createAboutBlankContentViewer\(null\)/, ''));
    -----------------------------------------------
 4. Press Ctrl-B to open the bookmarks sidebar panel.
    If it becomes hidden, press Ctrl-B again.
 5. Add many new bookmarks to make the sidebar panel scrollable.
 6. Scroll the sidebar panel to the bottom.
 7. Press Ctrl-B to collapse the sidebar.
 8. Press Ctrl-B to show the sidebar.
    Then, you'll see "scrolled to the bottom" sidebar panel again,
    because "collapsed" attribute doesn't break the contents.
 9. Copy following codes, paste them to the scratchpad, and run it.
    -----------------------------------------------
    var observer = new MutationObserver(function() { });
    observer.observe(document.createElement('box'), { childList : true });
    -----------------------------------------------
10. Press Ctrl-B to collapse the sidebar.
11. Press Ctrl-B to show the sidebar.

Actual result:
You see "scrolled to the top" sidebar panel. In other words, the scrolled state is lost.

Expected result:
You see "scrolled to the bottom" sidebar panel again.

Additional note:
Even if just one addon uses the MutationObserver, all other addons with collapsed inline frames will be affected. For example, All-in-One Sidebar.
https://addons.mozilla.org/firefox/addon/all-in-one-sidebar/

Comment 1

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

Any chance for a minimal testcase.
(And by looking at the code having just a dummy MutationObserver could only change 
CC/GC behavior)

Comment 2

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

But I can reproduce the problem. Really strange.

Comment 3

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

Actually, the scrolled state is there, but then something causes bookmarks to scroll up..

Comment 4

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

Ok, this is a XUL bug. It can't handle the case when attribute value doesn't change, yet
attribute setting is notified.

Comment 5

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

Or, hmm, something is setting .src to the same value as it is.
In XUL (if there are no mutation observers) we don't load anything in that case, but
in HTML we do.

Comment 6

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

I'll do something hackish here.

Comment 7

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

Attachment: patch

This is a big ugly. This basically makes sure that mutationobservers
don't affect how SetAttr/BeforeSetAttr/AfterSetAttr get called - 
only nsIMutationObserver stuff gets called like it has been all the time
we've had nsIDOMMutationObserver.
mozAutoDocUpdate between the calls is odd, but that is there just to keep
the existing behavior.

Comment 8

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

(I need to figure out some other approach when I make SetAttr non-virtual)

Comment 9

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

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

Review of attachment 648872 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsGenericElement.cpp
@@ +1878,5 @@
>        bool valueMatches = aValue.EqualsAsStrings(*info.mValue);
>        if (valueMatches && aPrefix == info.mName->GetPrefix()) {
>          if (OwnerDoc()->MayHaveDOMMutationObservers()) {
> +          nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName,
> +                                           nsIDOMMutationEvent::MODIFICATION);

Why are you only doing this if you have mutation observers? Isn't the problem here that our internal AttributeWillChange/Changed notifications aren't always called? So we should make sure to always call them, even when bailing early from SetAttr/SetParsedAttr.

I.e. wouldn't a better fix to move these new calls to the two

if (MaybeCheckSameAttrVal(...)) {
  // put new code here.
}

callsites?

That'd also keep this function more true to its name.

Comment 10

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

(In reply to Jonas Sicking (:sicking) from comment #9)
> Why are you only doing this if you have mutation observers? Isn't the
> problem here that our internal AttributeWillChange/Changed notifications
> aren't always called?

The problem is that nsMutationReceiver::AttributeWillChange isn't called

> So we should make sure to always call them, even when
> bailing early from SetAttr/SetParsedAttr.
> 
> I.e. wouldn't a better fix to move these new calls to the two
> 
> if (MaybeCheckSameAttrVal(...)) {
>   // put new code here.
> }
> 
> callsites?
> 
> That'd also keep this function more true to its name.
I could do that.

Comment 11

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

Though, it is a bit ugly to do the same thing twice, so I'll add a helper method.

Comment 12

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

Attachment: v2

This is a bit slower in cases there are no mutationobservers



https://tbpl.mozilla.org/?tree=Try&rev=f415d9dbfc64

Comment 13

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

Comment on attachment 651652 [details] [diff] [review]
v2

Something wrong, there are test failures.

Comment 14

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

Looks like we can't notify AttributeChanged if it didn't actually change.
But as the documentation says, it is ok to notify AttributeWillChange.

Comment 15

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

Uh, except that some code seems to rely that the method calls come in pairs, which is not true atm.

Comment 16

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

Attachment: safe approach

Comment 17

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

Comment on attachment 651788 [details] [diff] [review]
safe approach

Review of attachment 651788 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

But why do you need the new AttributeSetToCurrentValue notification? You don't appear to be using it anyway. If it's not needed I'd prefer to stick to just calling AttributeWillChange. We have too many attribute notifications as it is (though this doesn't add more function calls, just more types of notifications.)

Comment 18

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

it is used in dommutationobserver.
We can't reuse the existing notifications since their callbacks expect that attr value has changed...
so instead of fixing those impls I decided to go with the safe road and add a new notification.

Comment 19

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

Ah, I see now. Really bummed that we have to have yet another notification hook :(

Why was it decided that same-attr-value modifications should notify MutationObservers?

I ran numbers a long long time ago which showed that setting attributes to the same value was actually pretty common on complex pages like gmail. But that data might be obsolete by now.

Comment 20

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

(In reply to Jonas Sicking (:sicking) from comment #19)
> Why was it decided that same-attr-value modifications should notify
> MutationObservers?
Because of API simplicity and consistency.

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/1340cabb01d1