Closed Bug 662242 Opened 13 years ago Closed 13 years ago

Cannot access App Tab iframe.document in tab from session restore after FF upgrade

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox6 + fixed

People

(Reporter: kdevel, Assigned: dholbert)

References

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110605 Firefox/7.0a1

After a software upgrade an active app-tab application can no longer access the document-property of an (empty) iframe.

Only those upgrades are affected, which have old version less than and target version greather equals revision e9c7616c4f72.

http://hg.mozilla.org/mozilla-central/rev/e9c7616c4f72
"Bug 308590 patch 2: Move GetRef/SetRef from nsIURL to nsIURI. r=bz sr=biesi"

Reproducible: Always

Steps to Reproduce:
STR will be handed in later.

Actual Results:  
"Error: Permission denied to access property 'document'"

Expected Results:  
Allow access to property 'document' even after a software upgrade.
Attached file Testcase (obsolete) —
Attachment #537517 - Attachment mime type: text/plain → text/html
Steps to Reproduce:

1. start FF 4.0.1 (or any other version > 4.0.1 and < rev.e9c7616c4f72)
2. open Testcase url, pin tab as app tab
3. close FF
4. perform update to FF >= rev.e9c7616c4f72, start new FF
5. click at the 'A'-button

Actual Results:
- no "success" string
- Error: Permission denied to access property 'document'
Source File: https://bug662242.bugzilla.mozilla.org/attachment.cgi?id=537517
Line: 10

Expected Result:
"success" message
This does not seem to have any correlation whatsoever to Session Store. Moving into Firefox::General until a more appropriate component can be found.
Component: Session Restore → General
QA Contact: session.restore → general
Summary: app tab: access permission to empty iframe.document denied after FF upgrade → Cannot access App Tab iframe.document after FF upgrade
Does this only fail in the upgrade case? This is working just fine for me when restarting latest Nightly.
(In reply to comment #4)
Yes. You need to perform an upgrade from and to the abovementioned versions in order to reproduce. (Downgrade works, too).
Attached file validated Testcase
Attachment #537517 - Attachment is obsolete: true
Attachment #537650 - Attachment mime type: text/plain → text/html
dholbert - you wrote the patch being blamed... any ideas?
I'm guessing that when we're in Firefox 4.0, we serialize URIs in the document (to disk cache), and then when we deserialize them, we get an equality failure when comparing an deserialized URI from Firefox 4 vs. a freshly-parsed URI from Firefox 5.

And as a result of that URI equality failure, we prevent modifications to the iframe, as a security provision.  ("Error: Permission denied to access property 'document'")

Or something like that.  I can look in more detail in a bit.
Blocks: 308590
Status: UNCONFIRMED → NEW
Component: General → Networking
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → networking
Hardware: x86_64 → All
Version: unspecified → Trunk
(In reply to comment #8)
> comparing an deserialized URI from Firefox 4 vs. a
> freshly-parsed URI from Firefox 5.

sorry, s/Firefox 5/Trunk or Aurora/. (Bug 308590's patches aren't in Firefox 5, though they are in Trunk & Aurora)
cc-ing some HTTP cache folks.  From IRC chat seems like a likely solution is to bump nsDiskCache::kCurrentVersion.  Which is a heavy hammer--it'll blow away all user's disk cache when they upgrade :( --but perhaps the only way to fix.
Component: Networking → Networking: Cache
QA Contact: networking → networking.cache
Unless we can add support for deserializing v4 URIs into v5 ones?  And/or making them equal to "freshly parsed" v5 uri (not sure of the details here).
> 4. perform update to FF >= rev.e9c7616c4f72,
FWIW, that cset didn't actually change any functionality, really -- it effectively added a (unused) method on nsIURI, and revved the uuids on nsIURI and nsIURL.

Maybe revving the uuids is what broken this?
I would expect that revving the CID and serialization code of nsSimpleURI is a more likely culprit, but that's a different changeset.

In fact, that patch would have made principal deserialization fail, which would have made the iframe in the attached testcase have a different principal from the main frame.

There's no good workaround here.  I thought about the issue when reviewing the nsSimpleURI changes, but there was just no way to keep the serialization compatible-enough because the format is so rigid.  And the CID change, of course, kills the deserialization dead...
Note that I suspect that this is wholly unrelated to the HTTP cache.
Where does the URI serialization come into play here? I didn't think sessionstore serialized URIs (AFAIK it just uses .spec).
It serializes principals, I thought, and principal serialization serializes the URI of the principal...
(In reply to comment #14)
> Note that I suspect that this is wholly unrelated to the HTTP cache.

Hrm... a local nsDiskCache::kCurrentVersion-incrementing patch says that you're right about that.  (Incrementing the cache version doesn't seem to fix this.)

I'd initially thought this was cache-related because shift-reloading the app-tab in a mozilla-central build does seem to fix the problem.  But I guess that assumption was off-base...
zpao points to http://hg.mozilla.org/mozilla-central/annotate/f4321cdcef37/browser/components/sessionstore/src/nsSessionStore.js#l1771 as sessionstore code that could be influenced by nsIURI serialization differences.
Yes, see comment 16.

I'm sort of sad we're still using the quick hack we put in place to serialize principals/URIs via nsISerializable.  It's really not designed for changes where you want to make sure you preserve information: the original design is for a cache, where just blowing the cache away when the object layout changes is fine...
Thinking about this a bit more...  For the specific case of _upgrading_, we could perhaps make this work with some hackery.  Create a new class called nsCompatSimpleURI that has the same CID as the old nsSimpleURI CID, have it inherit from nsSimpleURI, and have it override the classinfo bits to return the different CID and override Read() to do what the old nsSimpleURI::Read did plus some sane-ish init of the new members.

For downgrading, that won't fly, though.
(In reply to comment #12)
> > 4. perform update to FF >= rev.e9c7616c4f72,
> FWIW, that cset didn't actually change any functionality, really
[...]
> Maybe revving the uuids is what broken this?

I verified that this quoted chunk is correct (with s/broken this/broke this/ :))  That is -- the interface UUID changes are what trigger the bustage.

Specifically:
 (A) A targeted build at rev e9c7616c4f72 is broken.
 (B) The same build with the UUIDs reverted is *not* broken.
(In reply to comment #20)
> Thinking about this a bit more...  For the specific case of _upgrading_, we
> could perhaps make this work with some hackery.  Create a new class called
> nsCompatSimpleURI that has the same CID as the old nsSimpleURI CID

For the purpose of fixing this bug's specific testcase, the above fix isn't actually required -- the nsIURI in that we fail to deserialize here is:
  https://bug662242.bugzilla.mozilla.org/attachment.cgi?id=537650
...which is a nsStandardURL, not a nsSimpleURI. (and the CID of nsStandardURL has not changed.)

However, I think we do need bz's proposed nsCompatSimpleURI for e.g. data URIs or "about:" URI app-tabs to deserialize correctly.
The only time we need URI deserialization is for principals, so I suspect it's not a big deal.
This patch is sufficient to fix this bug's testcase.  (though as noted in previous comment, we'll need something more to fix an equivalent-but-non-URL testcase)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #537895 - Flags: review?(bzbarsky)
Comment on attachment 537895 [details] [diff] [review]
patch 1: iid switcheroo in nsBinaryInputStream::ReadObject.

Please add a comment in nsIURI.idl pointing to this code?  r=me with that.
Attachment #537895 - Flags: review?(bzbarsky) → review+
(In reply to comment #22)
> However, I think we do need bz's proposed nsCompatSimpleURI for e.g. data
> URIs or "about:" URI app-tabs to deserialize correctly.

bz points out on IRC (clarifying comment 23) that data URIs actually get a nsNullPrincipal, not some principal involving a nsSimpleURI, so they won't be bitten by this.

"about:" URIs vary on what sort of principal they get, and it's possible that some addon could create an about: URI that would hit this bug when restored from sessionstore.  (though this would only be a problem if the about: URI in question contained iframes, and those iframes use the same principal as the about: page itself)

This case seems somewhat contrived/unlikely... and even if it happens, there are of course easy workarounds that permanently fix the issue (shift-reload, or focus-urlbar-and-hit-enter, or reopen the URI in a new tab).  So I tend to think it's not worth adding the nsCompatSimpleURI workaround to fix this low-likelihood edge case.
Added a comment to nsIURI, and also tweaked the added code chunk to use NS_IURI_IID in place of the hardcoded current-IID.

Re-requesting review to be sure the comment is ok & that you're ok with the NS_IURI_IID code-change.
Attachment #537895 - Attachment is obsolete: true
Attachment #537908 - Flags: review?(bzbarsky)
Filed bug 662693 on serializing nsPrincipal's member-nsIURI-vars as nsISupports instead of as nsIURI, to prevent future instances of this bug. (bz, correct me if I'm mistaken about that)
Per gavin's note in IRC, I'm tweaking the nsIURI comment to blame nsPrincipal URI-serialization (and bug 662693) rather than sessionstore.
Attachment #537908 - Attachment is obsolete: true
Attachment #537908 - Flags: review?(bzbarsky)
Attachment #537919 - Flags: review?(bzbarsky)
Comment on attachment 537919 [details] [diff] [review]
patch 1 v2a: IID switcheroo in nsBinaryInputStream::ReadObject

You don't need the newURIiid temporary.  r=me with that.
Attachment #537919 - Flags: review?(bzbarsky) → review+
(In reply to comment #30)
> You don't need the newURIiid temporary.

That's what I thought at first too, but it won't compile if I remove the temporary var.

NS_IURI_IID is a bracketed list like { 0xabc { 0x123 } }, which gets treated specially.  So the following three simplified lines all cause compile errors:
> iid = NS_IURI_IID;
> iid = nsIID(NS_IURI_IID);
> const nsIID newURIiid(NS_IURI_IID);
...since both nsID::operator= and nsID::nsID() reject bracketed lists as an input.

AFAICT, only this will compile:
> const nsIID newURIiid = NS_IURI_IID;

Correct me if I'm missing something, though. :)
Ah, ok.  Never mind that comment, then!
Fixed on trunk:
http://hg.mozilla.org/mozilla-central/rev/72400ce3811c

Needs fixing on aurora; requesting approval in a second.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Attachment #537919 - Flags: approval-mozilla-aurora?
Overview of patch, for approval purposes:
 - Reward: fixes possible issues with restoring a pre-Firefox6 session after upgrading to Firefox6.
 - Risk: Specifically checks for the old nsIURI IID, when de-serializing an object, and replaces it with the new one.

The nsStandardURL CID (and structure) hasn't changed, so we'll still be creating nsStandardURL instances, and deserializing them.  It's just that one of the interfaces (the one we care about QI'ing the created object to in this case) has had its IID revved, and we need to QI to the *new* interface ID for the deserialization to succeed.

This is a semi-ugly hack, but it's very targeted and hopefully avoidable in the future once bug 662693 is fixed.  (and it's kinda necessary for sessionstore to work through an upgrades)
(In reply to comment #34)
>  - Risk: Specifically checks for the old nsIURI IID, when de-serializing an
> object, and replaces it with the new one.

(sorry, that's more of a summary than a risk level.  I meant "risk: low, imho". :))
Attachment #537919 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Sorry, lost track of this.

Landed on aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/903365ff5efa
OS: Linux → All
Summary: Cannot access App Tab iframe.document after FF upgrade → Cannot access App Tab iframe.document in tab from session restore after FF upgrade
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0 beta5
Status: RESOLVED → VERIFIED
Regression. In step #1 take 2011-08-01-03-09-16-mozilla-central
amd in         step #4 take 2011-08-21-03-07-58-mozilla-central
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
I have no idea what comment 38 is about, but it should be in some other (possibly new) bug.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
@Stefan please file a new bug describing the issue you are seeing with that regression range. Thanks
Bug 682031 Submitted
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
@Stefan no need to reopen this bug once you file a new one. Thanks
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
@Anthony Hughes the status changed automatically when I submitted comment #41.
(yup that's Bugzilla being silly, not your fault)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: