874321 - Changing the value of an expando on an Xray for a DOM proxy binding does not work

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

To the point of being unusable, the latest Aurora 23.0a2, just released May 20th is unusable with something like zimbra e-mail.  Beta is fine.

Comment 1

[Joe Drew (not getting mail)]

What do you mean by "small images regression"?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Running in -safe-mode, the regression goes away.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

This goes away when Telify 1.3.3 add-on is disabled.

Comment 4

[Georg Fritzsche [:gfritzsche]]

From what i gathered that extension just uses protocol handlers like tel, callto, skype, ... and does not seem to embed any plugins (extensions != plugins).

Jorge, do you think it is worth reaching out to the author here?

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

I think it'd be good to figure out if this is some sort of regression. Finding the reason would help me reach out to the developer with useful information.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll see if I can get a regression range.  I did not modify the add-in, and it went from running fine to being unusable as Aurora got updated.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Last good nightly: 2013-05-05
First bad nightly: 2013-05-06
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8e47b184aba&tochange=b109e2dbf03b

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

This is a result of fixing bug 855971 - http://hg.mozilla.org/mozilla-central/rev/adaaf6641785.  Peter, it would probably be good to understand if the add-on was doing something wrong and was getting away with it before, or if we need to treat this as a regression and the add-on is just a messenger.
The STR:
1. Install Telify 1.3.3, restart
2. Go to https://mail.mozilla.com
3. It either takes a couple of seconds (good), or 30+ (bad).

Comment 9

[Peter Van der Beken [:peterv]]

So far I haven't been able to reproduce on a trunk build on OS X. https://mail.mozilla.com always loads in a couple of seconds for me.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I can definitely reproduce this with the May 21 nightly on OSX.

I _think_ the relevant profile is http://people.mozilla.com/~bgirard/cleopatra/#report=f59875e83279d67ec0b943dc962f4b633b5d0ba5

This is showing a ton of time spent firing a timeout that calls "objTelify.onDOMModified/<() @ telify.js:874" which is the anonymous function that onDOMModified posts off a timeout.

now the telify scripts have this bit in several places:

  try {
    if (node.contentDocument.observer == undefined) {
      node.contentDocument.observer = new MutationObserver(function(mutations)
                                                         {objTelify.onDOMModified(null)});
      node.contentDocument.observer.observe(node.contentDocument, {subtree: true});
    }
  } catch (e) {
    node.contentDocument.addEventListener("DOMSubtreeModified", objTelify.onDOMModified,
                                          false);
  }

so if for some reason that if block throws (which it does, in my debug build), then this code will fall back to using a mutation listener.  Which will make it walk the entire DOM on every mutation, as opposed to on every batch on mutations.  And then things will be unbearably slow.  Note that Zimbra does a _lot_ of DOM mutations as it starts up.

Looking now into why the try/catch ends up throwing.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Also, if the new MutationObserver happens to be what throws this code would add one mutation listener every time through, which is just terrible.  I don't _think_ that's what happens here, but checking.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, what throws here is the node.contentDocument.observer.observe() call:

455       if (!(aOptions.mChildList || aOptions.mAttributes || aOptions.mCharacterData)) {
456         aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

and this is clearly not passing any of those three options.

But that should not have changed with the HTMLDocument changes...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ah, so.  onDOMModified tries to deal with the "multiple calls" issue from DOMSubtreeModified listeners like so:

	if (content.document.tel_modified == undefined) {
		content.document.tel_modified = 1;
	} else {
		content.document.tel_modified++;
	}
	if (content.document.tel_modified > 1) return;

and then in the timeout handler it posts:

			content.document.tel_modified = 0;

So I added logging like so:

  dump("BEFORE: " + content.document.tel_modified + "\n");
  if (content.document.tel_modified == undefined) {
    content.document.tel_modified = 1;
  } else {
    content.document.tel_modified++;
  }
  dump("AFTER: " + content.document.tel_modified + "\n");

and here's what I get on Zimbra: 

  BEFORE: 0
  AFTER: 0
  BEFORE: 0
  AFTER: 0

which of course is not helpful for avoiding doing the work tons of times.  ;)

Looking into why the ++ there is not working, but that seems at least related to the HTMLDocument changes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

A simple testcase to reproduce.  

1)  Make sure devtools.chrome.enabled is true in about:config
2)  Tools > Web Developer > Scratchpad
3)  Environment > Browser
4)  Type in the following:

     var doc = content.document;
     doc.foo = 5;
     doc.foo = 7;
     doc.foo

5) Execute > Display

This shows 5, when it should of course show 7.  That's _definitely_ a bug in the new binding/Xray stuff.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Peter can you repro now with the reduced testcase?  Would be great to get a fix for this before we go to Beta in a few weeks with 23.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So we get into xpc::DOMXrayTraits::resolveOwnProperty and call XrayTraits::resolveOwnProperty.  This comes back with a non-null desc->obj (that being our |wrapper|), so we return immediately.  So far everything is fine: the desc claims we have an enumerable writable value property, etc.

Now we call xpc::DOMXrayTraits::defineProperty 

Then we land in mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::defineProperty.  This checks for the property on the document (which is already a bit questionable, coming from an Xray!) and calls mozilla::dom::DOMProxyHandler::defineProperty.

In this last, xpc::WrapperFactory::IsXrayWrapper(proxy) is true, so we return true without doing anything.

So this is just totally broken for proxy bindings.

The question is: how _should_ this work, exactly?

Comment 17

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #16)
> So we get into xpc::DOMXrayTraits::resolveOwnProperty and call
> XrayTraits::resolveOwnProperty.  This comes back with a non-null desc->obj
> (that being our |wrapper|), so we return immediately.  So far everything is
> fine: the desc claims we have an enumerable writable value property, etc.
> 
> Now we call xpc::DOMXrayTraits::defineProperty 

This is where things go wrong. DOMXrayTraits::defineProperty sets defined to true (I think because there's no way for the handler's defineProperty to signal that it defined something). We could make the handler's defineProperty define properties on the Xray expando, but I'd rather keep that in the Xray code. So we need to signal from the handler to the Xray code that we've not defined the property.

Comment 18

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Then we land in
> mozilla::dom::HTMLDocumentBinding::DOMProxyHandler::defineProperty.  This
> checks for the property on the document (which is already a bit
> questionable, coming from an Xray!)

Why is it questionable? I do think we want the same behaviour for named properties whether we come through an Xray or not.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Why is it questionable?

It's questionable because as far as I can tell it means this addon will break if the document has an <img id="tel_modified">, because setting .tel_modified on it will throw, no?  That's pretty surprising behavior for an Xray.

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 754743 [details] [diff] [review]
v1

r=me.  Very nice.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Actually, shouldn't DOMProxyHandler::defineProperty set *defined to something?  Or is the contract that it's preinitialized to false?  If the latter, we should document it somewhere.

Comment 23

[Peter Van der Beken [:peterv]]

Xray code sets it to false. The handlers normal defineProperty hook doesn't initialize it, but it's unused in that case. I'll add a comment to XrayDefineProperty and the new defineProperty that it only needs to be set if it should be true.

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: Fix compilation

Turns out I also need this for builds that have "-Werror=overloaded-virtual". Not sure that we really need ClassUsingDeclaration, we could just use extradeclarations for the few times that we run into this.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 755633 [details] [diff] [review]
Fix compilation

r=me

Comment 26

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2454694ed4

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b2454694ed4

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: Combined patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 855971
User impact if declined: add-on breakage
Testing completed (on m-c, etc.): landed on m-c, has a test
Risk to taking this patch (and alternatives if risky): low-risk
String or IDL/UUID changes made by this patch: none

Comment 29

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d6df86bb3dc