Shouldn't change userContextId attr of a <xul:browser> once it has loaded a document or the usercontextId in the docshell

RESOLVED FIXED in Firefox 51

Status

()

Core
Security
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: allstars, Assigned: baku)

Tracking

(Blocks: 1 bug)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

Details

(Whiteboard: [userContextId][OA])

Attachments

(1 attachment, 3 obsolete attachments)

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>
Whiteboard: [userContextId]
Priority: -- → P1
This bug will add an assertion to ensure this doesn't happen.  But this probably refers to tab:browser, not xul:browser.
Whiteboard: [userContextId] → [userContextId][OA]
Change subject to *tabbrowser* and CCing Jonas.
Summary: Shouldn't change userContextId attr of <xul:browser> once it has loaded a document → Shouldn't change userContextId attr of a tabbrowser once it has loaded a document
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Summary: Shouldn't change userContextId attr of a tabbrowser once it has loaded a document → Shouldn't change userContextId attr of a tab once it has loaded a document
Whiteboard: [userContextId][OA] → [userContextId]
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.
Whiteboard: [userContextId] → [userContextId][OA]
Iteration: --- → 49.3 - Jun 6
(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).
Created attachment 8753721 [details] [diff] [review]
WIP. Patch

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

Updated

2 years ago
Summary: Shouldn't change userContextId attr of a tab once it has loaded a document → Shouldn't change userContextId attr of a <xul:browser> once it has loaded a document
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
Attachment #8753721 - Flags: feedback?(jonas)
Assignee: allstars.chh → jhao

Comment 7

2 years ago
Change assignee according to offline discussions.
Assignee: jhao → amarchesini
See Also: → bug 1271516
Comment on attachment 8753721 [details] [diff] [review]
WIP. Patch

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

Baku has a better way to do it.
Attachment #8753721 - Flags: feedback?(jonas)
(Reporter)

Updated

2 years ago
Attachment #8753721 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
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.
Flags: needinfo?(jonas)
Summary: Shouldn't change userContextId attr of a <xul:browser> once it has loaded a document → Shouldn't change userContextId attr of a <xul:browser> once it has loaded a document or the usercontextId in the docshell
(Assignee)

Updated

2 years ago
Flags: needinfo?(jonas) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 11

2 years ago
Created attachment 8782806 [details] [diff] [review]
check.patch
Attachment #8782806 - Flags: review?(bugs)
(Assignee)

Comment 12

2 years ago
Created attachment 8782807 [details] [diff] [review]
check.patch
Attachment #8782806 - Attachment is obsolete: true
Attachment #8782806 - Flags: review?(bugs)
Attachment #8782807 - Flags: review?(bugs)
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
Attachment #8782807 - Flags: review?(bugs) → review-
(Assignee)

Comment 14

2 years ago
Created attachment 8783604 [details] [diff] [review]
check.patch
Attachment #8782807 - Attachment is obsolete: true
Attachment #8783604 - Flags: review?(bugs)
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+
Attachment #8783604 - Flags: review?(bugs) → review+

Comment 16

2 years ago
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

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/702ab78cea26
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
status-firefox48: affected → wontfix
status-firefox49: --- → wontfix
status-firefox50: --- → wontfix
You need to log in before you can comment on or make changes to this bug.