See https://bugzilla.mozilla.org/show_bug.cgi?id=1250063#c40 <quote> * Thus we should never change the userContextId attribute of a <xul:browser> which we've ever loaded a document into. </quote>

This bug will add an assertion to ensure this doesn't happen. But this probably refers to tab:browser, not xul:browser.

Change subject to *tabbrowser* and CCing Jonas.

This affects OriginAttributes as well. It means that there's code somewhere that is trying to change the OriginAttributes of an existing document, which is something that simply doesn't work.

(In reply to Tanvi Vyas [:tanvi] from comment #1) > This bug will add an assertion to ensure this doesn't happen. But this > probably refers to tab:browser, not xul:browser. Checking XUL again, the <browser> tag is under XUL namespace, so it should be <xul:browser>, for example, each tab has its own <xul:browser> (through linkedBrowser property).

I tried to add a method in <browser> However it seems not working in _createBrowser from tabbrowser.xml

Comment on attachment 8753721 [details] [diff] [review] WIP. Patch Review of attachment 8753721 [details] [diff] [review]: ----------------------------------------------------------------- Hi Jonas I'd like to get some early feedback from you while I am studying the XUL/XBL bindings. :P So far for the xul:browser, we add call setAttribute("usercontextid", uid); on it, my idea is to replace those calls with a setUserContextId method in <xul:browser>, and when the method is called, we assert the contentDocument is null. However please see the TODO in my patch, b.setUserContextId will throw with "setUserContextId is not a function." I thought document.createElementNS(NS_XUL, "browser"); should create a <xul:browser> element? Or I was wrong in somewhere? Thanks

Change assignee according to offline discussions.

Comment on attachment 8753721 [details] [diff] [review] WIP. Patch Review of attachment 8753721 [details] [diff] [review]: ----------------------------------------------------------------- Baku has a better way to do it.

I think we should not allow the changing of originAttributes to a docShell after the first loading. This *should* the correct approach to this issue. The UI is important, of course, but the protection should go deeper into the docShell.

Not sure what I'm supposed to say here :) I think we should MOZ_ASSERT in nsXULElement::BeforeSetAttr if one is trying to replace or remove an existing usecontextId attribute. (In remove case aValue is null) Also, I think we should at least eventually assert also in case one tries to set the attribute when element is already in non-data document. (since that means trying to replace default uci with something else). docshell code just shouldn't accept (should MOZ_ASSERT in debug builds) changes to OA after it has been initialized - so OA should be set before docshell has _any_ document, even the about:blank.

Comment on attachment 8782807 [details] [diff] [review] check.patch >+ } else if (aNamespaceID == kNameSpaceID_None && >+ aName == nsGkAtoms::usercontextid) { odd indentation. aName should be below aNamespaceID >+ nsAutoString oldValue; >+ GetAttr(kNameSpaceID_None, nsGkAtoms::usercontextid, oldValue); >+ if (!oldValue.IsEmpty() && aValue && You want to test the existence of the attribute, not only its value. So bool hasAttribute = GetAttr(kNameSpaceID_None, nsGkAtoms::usercontextid, oldValue); if (hasAttribute && aValue && ...) >+ !aValue->String().Equals(oldValue)) { >+ MOZ_CRASH("Changing usercontextid is not allowed."); >+ return NS_ERROR_INVALID_ARG; >+ } >+ >+ if (!aValue && !oldValue.IsEmpty()) { This should also use hasAttribute

Comment on attachment 8783604 [details] [diff] [review] check.patch Crap, I managed to remove part of my previous comment. So, don't use MOZ_CRASH. That would make totally valid DOM API to crash in certain cases, even in opt builds. Make it MOZ_ASSERT, so that only debug build fails. _With_that_, r+

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/702ab78cea26 XULElement should not allow the changing of usercontextid attribute, r=smaug