389274 - [FIX]Midas demo. (designMode) fails to work properly after restoring with session restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Adam Guthrie]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5

This is spun off from bug 385882.

Steps to reproduce:
1. Tools > Options > Main > Choose "Show my windows and tabs from last time"
2. Go to http://www.mozilla.org/editor/midasdemo/ and input some text.
3. File > Quit
4. Reopen Firefox and try typing into the editor.

Actual results:
The text that was in the editor is NOT restored and you cannot type anything in the editor. An error is also in the Error Console:

Error: uncaught exception: Permission denied to set property HTMLDocument.designMode

Expected results:
Text is restored and I'm able to edit it. 

This regressed on BRANCH between 2007-07-11-03 and 2007-07-12-03: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-11+02&maxdate=2007-07-12+04&cvsroot=%2Fcvsroot.

This issue has been around on trunk since at least 2007-01-01-04, FWIW.

Comment 1

[Martijn Wargers (dead)]

Confirmed, I see this in the error console:
Error: uncaught exception: Permission denied to set property HTMLDocument.designMode

Comment 2

[Adam Guthrie]

Aha! So I tracked down the regression range on trunk to between 2006-08-15-04 and 2006-08-16-04. Bug 332182 landed in this range as well as in the branch regression range I found, so I guess we've found our offender.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is a bug in session restore, more or less.  Its serialization and deserialization of history entries doesn't include the principal (exposed as .owner on the history entry since bug 337260 got fixed).  As a result, when it loads that session history (which seems to be how it handles restoration as far as I can tell), the permission end up wrong.  It's always had problems for data: and javascript: URIs; all that happened is that about:blank now has the same problems.

Chances are, just serializing out the URI of the principal and then creating a codebase principal when deserializing will "work" on branch in most cases.  On trunk, I've been working on making principals implement nsISerializable.  I assume that session restore can store binary blobs as needed?

I'd be happy to help out if needed, but I'd want to be pointed at the places where we do the actual serializing and deserializing in nsSessionStore.js.  Then we'd want to change _deserializeHistoryEntry to use create() like docshell does itself.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Then again, brendan's been arguing for an alternate serialization format for principals.  If people can figure out what they want, we could do that too.

Comment 5

[Simon Bünzli]

(In reply to comment #3)
> I assume that session restore can store binary blobs as needed?

Since we support JSON serialization, you'd have to manage with strings, IEEE 754 numbers and arrays containing those. An encoded string should probably do the job.

> where we do the actual serializing and deserializing in nsSessionStore.js. 

That's really just _serializeHistoryEntry and _deserializeHistoryEntry which convert a "@mozilla.org/browser/session-history-entry;1" to a JS object and vice versa.

Comment 6

[Daniel Veditz [:dveditz]]

Not blocking the 1.8 branch, but when there's a trunk fix please renominate for the branch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk fix

This patch requires the fix for bug 369566 to work.  It also fixes the bug the existing code had with truncating postdata at the first null, while I was here.

jst, could you review the session history part?

mconnor, could you review the session store part?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch patch

Doesn't really have much to do with the trunk patch, for what it's worth.

Comment 9

[Simon Bünzli]

Comment on attachment 280766 [details] [diff] [review]
Trunk fix

Thanks for taking this on, Boris.
My comment to the SessionStore bits:

>-        var postdata = stream.read(stream.available());

What's the reason for the seemingly unrelated postdata changes?

>+      catch (ex) { debug(ex); }

What are you expecting to catch here?

>-      stream.setData(aEntry.postdata, -1);
>+      var postdata = atob(aEntry.postdata);
>+      stream.setData(postdata, postdata.length);

This looks like it will break if there's postdata to be restored when Firefox 3 is started for the first time (having Firefox set to "restore the windows/tabs from last time").

I'd prefer if you used a different property (aEntry.postdata_b64 or similar) and fell back to Firefox 2's property if available.

BTW: What's the advantage of base64'ing the string? Aren't JavaScript strings able to contain \0 ?

>+      var binaryData = atob(aEntry.owner);

Again: what will happen here when aEntry.owner is a nsIURI.spec from Firefox 2?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> What's the reason for the seemingly unrelated postdata changes?

See comment 7.

> What are you expecting to catch here?

I have no idea.  I modeled this after the postdata code, basically.  The obvious thing would be the Write() method of the object failing for some reason.

> I'd prefer if you used a different property (aEntry.postdata_b64 or similar)

OK, I can do that.

> Aren't JavaScript strings able to contain \0 ?

They are, but the JSON serialization doesn't deal.  See bug 396068.  There might be similar issues with other control chars, so this seemed like the safe thing to do.

> Again: what will happen here when aEntry.owner is a nsIURI.spec from Firefox
> 2?

I suppose we can use ownerURI on branch and create a principal here... Would that work? 

Too bad our security APIs are such a mess.  :(

Comment 11

[Simon Bünzli]

(In reply to comment #10)
> I have no idea.

Catching seems like the safer thing to do, but please add a comment as to that this isn't anything special you're trying to catch (in case the code gets refactored or we want to try without the try-catch-block).

> They are, but the JSON serialization doesn't deal.  See bug 396068.

Hm, our JSON code would really have dealt with that more graciously than toSource which we use here. Fixing bug 387859 would thus prevent the need for this hack. Please add a comment as to considering reverting the postdata bits once that bug is fixed.

> I suppose we can use ownerURI on branch and create a principal here...

That'd be great. Any chance your trunk patch could make use of .ownerURI as well when .owner is missing?

BTW: How portable are the serialized owners (Mac/Windows/different profiles)?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Please add a comment as to considering reverting the postdata bits

Some of those bits are needed no matter what (e.g. the setData() length changes).  But yes, the base64 encoding would not be needed.

> Any chance your trunk patch could make use of .ownerURI as
> well when .owner is missing?

That's what I meant by "create a principal here".  ;)

> BTW: How portable are the serialized owners (Mac/Windows/different profiles)?

They're as portable as anything else we send through the binary input stream.  Which means that things will work unless someone changes the serialization functions in nsPrincipal, will be portable across different endianness machines, etc.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated branch patch

Let me know if I need a different reviewer, ok?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated trunk fix

I've tested that Fx2 -> Fx2, Fx2 -> trunk, trunk -> trunk restores all work with these patches.

Comment 15

[Simon Bünzli]

Comment on attachment 281133 [details] [diff] [review]
Updated trunk fix

r+ for the SessionStore changes. Just one detail:

>+          // We can stop doing base64 encoding once our serialization into JSON
>+          // is guaranteed to handle all chars in strings, including embedded
>+          // nulls.

JSON and toSource/uneval are two quite similar but still different serializations - and it's only the latter which fails here. What about "... once .toSource() is guaranteed ... (see bug 375639)."?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I can do that, sure.  Though I wouldn't trust it to deal once that bug is fixed....

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281132 [details] [diff] [review]
Updated branch patch

Requesting branch approval.  This should be a very safe fix that fixes corner cases of session restore and about:blank subframes in most cases (basically all cases except when the parent page has a certificate principal).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281133 [details] [diff] [review]
Updated trunk fix

Requesting approval for regression fix.  Risk is pretty low, in my opinion.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Trunk fix checked in.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 281132 [details] [diff] [review]
Updated branch patch

approved for 1.8.1.8, a=dveditz for release-drivers

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed on branch

Comment 22

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.8 using Mozilla/5.0 (Windows; U; Windows NT 5.2; de; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8 ID:2007100816 and the steps to reproduce from this bug