Closed Bug 874321 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed

People

(Reporter: milan, Assigned: peterv)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
Summary: Small images regression with 23.0a2, at least on the OSX (zimbra, among others) → Performance regression 23.0a2, at least on the OSX (zimbra, among others)
This goes away when Telify 1.3.3 add-on is disabled.
Component: Graphics → Plug-ins
Summary: Performance regression 23.0a2, at least on the OSX (zimbra, among others) → Performance regression 23.0a2 with Telify 1.3.3 add-on
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?
Component: Plug-ins → Add-ons
Product: Core → Tech Evangelism
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.
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).
Flags: needinfo?(peterv)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(peterv)
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.
Flags: needinfo?(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.
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.
Component: Add-ons → DOM
Product: Tech Evangelism → Core
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.
Assignee: nobody → peterv
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?
Flags: needinfo?(bobbyholley+bmo)
Summary: Performance regression 23.0a2 with Telify 1.3.3 add-on → Changing the value of an expando on an Xray for a DOM proxy binding does not work
(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.
Flags: needinfo?(bobbyholley+bmo)
(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.
Attached patch v1Splinter Review
Attachment #754743 - Flags: review?(bzbarsky)
Comment on attachment 754743 [details] [diff] [review]
v1

r=me.  Very nice.
Attachment #754743 - Flags: review?(bzbarsky) → review+
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.
Attached patch Fix compilationSplinter Review
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.
Attachment #755633 - Flags: review?(bzbarsky)
Comment on attachment 755633 [details] [diff] [review]
Fix compilation

r=me
Attachment #755633 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/4b2454694ed4
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Attached patch Combined patchSplinter Review
[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
Attachment #756509 - Flags: approval-mozilla-aurora?
Attachment #756509 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.