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.

What do you mean by "small images regression"?

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

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

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?

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.

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.

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

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).

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.

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.

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.

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...

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.

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.

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.

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?

(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.

(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.

> 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 on attachment 754743 [details] [diff] [review] v1 r=me. Very nice.

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.

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.

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 on attachment 755633 [details] [diff] [review] Fix compilation r=me

[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