1318303 - removeAttribute (or anything else that changes the "data" attribute) on an <object> in a data document will cause NS_ERROR_UNEXPECTED exception

Status: RESOLVED FIXED

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

Description

[myvyang]

Attachment: tmp.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36

Steps to reproduce:

as code in tmp.html attached, run normal in other browsers ,like IE, chrome, safari.

but in firefox, it broke JS and throws "NS_ERROR_UNEXPECTED: "


Actual results:

in firefox, tmp.html broke JS and throws "NS_ERROR_UNEXPECTED: "


Expected results:

should run normal

Comment 1

[Andrew Overholt [:overholt]]

Boris, is this expected behaviour or is Gecko doing something wrong? Should we align with other browsers?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Gecko is doing something wrong.

HTMLObjectElement::SetAttr does:

  if (aNotify && IsInComposedDoc() && mIsDoneAddingChildren &&
      aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::data) {
    return LoadObject(aNotify, true);
  }

and nsObjectLoadingContent::LoadObject does:

  // Sanity check
  if (!InActiveDocument(thisContent)) {
    NS_NOTREACHED("LoadObject called while not bound to an active document");
    return NS_ERROR_UNEXPECTED;
  }

Note the lack of check for "active document" in SetAttr, which means we hit that assertion and then return NS_ERROR_UNEXPECTED.  

Per spec, there should be no exception here, though we should fall back for the "not active document" case.  We should probably just remove the NS_NOTREACHED (since that's just not true) and change to returning NS_OK.  That said, this InActiveDocument check doesn't match the spec's concept of "active document", so maybe we should adjust that too while we're here?

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Fallback without exception when trying to load object in non-active document

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8818354 [details] [diff] [review]
Patch 1 (v1) - Fallback without exception when trying to load object in non-active document

r=me assuming there is no need to fall back here...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Kyle, is there a reason this didn't get landed?

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Was gonna try to add a test and figure out whether we should fallback, then got sidetracked. Am PTO-ish until next Tuesday, will look at it then.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

ni?'ing self so I remember to actually build test and land.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Added alternate fallback loading (treating comment #4 as nit) and mochitest. Try run: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1327e5296b656703b5b1d7009fb048307d99b053

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/563468e72885
Fallback without exception when trying to load object in non-active document; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3985fbac8369
Mochitest for object node data attribute removal; a=TEST-ONLY

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ddc15cef26
Backout 3985fbac8369 and 563468e72885 due to crashtest failures

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed24ffde1751
Fallback without exception when trying to load object in non-active document; r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3e618e8d95
Mochitest for object node data attribute removal; a=TEST-ONLY

Comment 12

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/ed24ffde1751
https://hg.mozilla.org/mozilla-central/rev/ce3e618e8d95